Skip to content

✨ Allow options to be updated on the MarkdownIt instance#35

Merged
hukkin merged 8 commits intomasterfrom
parse-env
Sep 30, 2020
Merged

✨ Allow options to be updated on the MarkdownIt instance#35
hukkin merged 8 commits intomasterfrom
parse-env

Conversation

@chrisjsewell
Copy link
Copy Markdown
Contributor

@chrisjsewell chrisjsewell commented Sep 25, 2020

This provides a hook for passing variables to the parsing/render process,
which in-turn provides a simple way of allowing plugins to accept variable arguments from the API or CLI.

Note this is back-incompatible for parser plugins, since update_mdit now also gets passed this env dict.

@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 25, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@ee503cd). Click here to learn what that means.
The diff coverage is 95.83%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #35   +/-   ##
=========================================
  Coverage          ?   96.24%           
=========================================
  Files             ?        9           
  Lines             ?      453           
  Branches          ?        0           
=========================================
  Hits              ?      436           
  Misses            ?       17           
  Partials          ?        0           
Impacted Files Coverage Δ
mdformat/_util.py 94.11% <92.85%> (ø)
mdformat/_api.py 92.85% <100.00%> (ø)
mdformat/_cli.py 94.02% <100.00%> (ø)
mdformat/plugins.py 87.50% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ee503cd...4f070ab. Read the comment docs.

@hukkin
Copy link
Copy Markdown
Owner

hukkin commented Sep 26, 2020

Hey, could you help me understand a few things.

We already pass some state to the parser and render in the options dict. Why not use that here as well? I'm having a bit of a hard time understanding the options and env distinction in the markdown-it world in general. I've only seen the env used for reference link refs, so thought that the use-case for that is parser output that for one reason or the other doesn't fit in the token stream. The definition here:

An env sandbox can be used alongside tokens to inject external variables for your parsers and renderers.

Makes me think that the source of env data is not always the parser. Then how is it different from options?

@hukkin
Copy link
Copy Markdown
Owner

hukkin commented Sep 26, 2020

Also would be interested in how you plan to use this feature exactly? Is there a use case for update_mdit receiving the env or is it enough if the render function does so, etc?

@chrisjsewell
Copy link
Copy Markdown
Contributor Author

chrisjsewell commented Sep 27, 2020

I'm having a bit of a hard time understanding the options and env distinction in the markdown-it world in general

The difference is that options should remain constant over multiple text parses, whereas env is transient and gets reset for each parse.
For example, you should set up your MarkdownIt object once then reuse it for all parsing:

mdit = MarkdownIt()
# this option wil be used for every parse
mdit.option["a"] = 1
# but the env will only be used for the parse it is set on
mdit.parse("hallo", env={"b": 1})
mdit.parse("hallo")

So yeh you are probably right that this data should update options.

Note, mdformat anyhow diminishes this distinction a bit, because a new MarkdownIt instance is created for every parse.
I guess technically, in mdformat._cli.run, you should set up a single MarkdownIt instance and use it to parse all files, rather than calling mdformat.text and creating a new one every time.
(this is more important in long running/responsive programs, for example VS Code sets up a single MarkdownIt instance in memory, and will use it for every call to update a preview)

Also would be interested in how you plan to use this feature exactly?

Any way I want lol.
If a plugin wants to be a bit more "unopinionated" (i.e. accepting configuration) then this should be allowed.
For an example though, markdown_it.extensions.texmath.texmath_plugin can be initialized with different styles of delimiter: dollars, brackets, gitlab, etc:

mdit.use(texmath_plugin, delimiters="gitlab")

I don't want to have to create a separate plugin for every style, over just calling something like mdformat -e texmath-delim=brackets:

mdit.use(texmath_plugin, delimiters=mdit.options.get("texmath-delim", "dollars"))

@chrisjsewell
Copy link
Copy Markdown
Contributor Author

chrisjsewell commented Sep 27, 2020

In 0672535 I've moved to updating mdit.options rather than using env

@chrisjsewell chrisjsewell changed the title ✨ Allow env dict to be passed to renderer ✨ Allow options to be updated on the MarkdownIt instance Sep 27, 2020
@hukkin
Copy link
Copy Markdown
Owner

hukkin commented Sep 27, 2020

Thanks for the explanation!

...how you plan to use this feature exactly?

Any way I want lol...

Aha, if it's as general purpose as this, then what do you think about the following: we give plugins the possibility to add argparse arguments of their own. Then in the render phase we'd pass all CLI args to the plugin. This way they the plugins can add their own --help strings to document the argument, argparse will raise if two plugins use the same name, etc.

I think we'd need another function to the plugin interface, something like

def add_options(parser: argparse.ArgumentParser) -> None:
    ...

which is where a plugin would add its options.

Then in mdformat._cli.run we could pass a mapping of the parsed args to the formatter function mdformat.text to make the options usable from Python API as well as the CLI:

import mdformat
options = vars(parser.parse_args(cli_args))
mdformat.text("bla bla", options=options)

@hukkin
Copy link
Copy Markdown
Owner

hukkin commented Sep 27, 2020

We could make defining add_options optional for the plugin by getting the function from the plugin with getattr and some default.

What do you think?

@chrisjsewell
Copy link
Copy Markdown
Contributor Author

What do you think?

Yeh absolutely, I thought you'd be more willing to accept a simpler approach lol, but that is more of a "complete" solution 👍

@chrisjsewell
Copy link
Copy Markdown
Contributor Author

Then in the render phase we'd pass all CLI args to the plugin

I think these options should still be stored in mdit.options though, potentially under a top-level key, e.g.

mdit = MarkdownIt()
options = vars(parser.parse_args(cli_args))
mdit.options["mdformat"] = options

@chrisjsewell
Copy link
Copy Markdown
Contributor Author

We could make defining add_options optional for the plugin by getting the function from the plugin with getattr and some default.

Yeh thts what I have done in #36, for allowing plugins to specify certain HTML classes to ignore when comparing the input/output HTML

@hukkin
Copy link
Copy Markdown
Owner

hukkin commented Sep 27, 2020

potentially under a top-level key

I would use a top-level key too. Not sure, but perhaps even two keys, if we want to fit the current "codeformatters" and "parser_extensions" in the same top-level key. Something like:

options["mdformat"]["opts"]
options["mdformat"]["codeformatters"]
options["mdformat"]["parser_extensions"]

This provides a hook for passing variables to the parsing/render process,
which in-turn provides a simple way of allowing plugins to accept variable arguments from the API or CLI .
Comment thread mdformat/_api.py
Comment thread mdformat/_cli.py Outdated
Comment thread mdformat/_cli.py Outdated
Comment thread mdformat/_cli.py Outdated
Comment thread mdformat/plugins.py Outdated
Comment thread mdformat/plugins.py Outdated
Comment thread mdformat/_api.py Outdated
Comment thread mdformat/_cli.py Outdated
Comment thread mdformat/_cli.py Outdated
Comment thread mdformat/plugins.py Outdated
Comment thread mdformat/plugins.py Outdated
Comment thread mdformat/plugins.py Outdated
@hukkin
Copy link
Copy Markdown
Owner

hukkin commented Sep 29, 2020

Looking very good! There's one unresolved comment I think you might have missed.

@chrisjsewell chrisjsewell requested a review from hukkin September 30, 2020 03:40
@chrisjsewell
Copy link
Copy Markdown
Contributor Author

yeh I just had to fix the test

@hukkin hukkin merged commit 6d5683f into master Sep 30, 2020
@hukkin
Copy link
Copy Markdown
Owner

hukkin commented Sep 30, 2020

Beautiful, thanks! Merged

@hukkin hukkin deleted the parse-env branch September 30, 2020 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants