👌 IMPROVE: Stop using YAML frontmatter-based directive options #49
👌 IMPROVE: Stop using YAML frontmatter-based directive options #49brunobeltran wants to merge 3 commits into
Conversation
Instead, we move to the more modern recommendation (per the MyST docs) of using the "colon" syntax for directive options.
ac53aba to
f5a3bd5
Compare
KyleKing
left a comment
KyleKing
left a comment
There was a problem hiding this comment.
For the review, I took a look at: brunobeltran/mdformat-myst@bruno/colon-fence-for-upstream...brunobeltran:mdformat-myst:bruno/use-modern-option-syntax
| LOGGER.warning("Invalid YAML in MyST directive options.") | ||
| return raw_content | ||
| if parsed: | ||
| formatted += "\n".join([f":{k}: {v}" for k, v in parsed.items()]) + "\n\n" |
There was a problem hiding this comment.
Would it make sense to have a way for people to opt-out of the newer formatting with a configuration option or do you think most people would prefer these changes?
|
Just following up, I would be happy to approve and mere, but I don't have the context on myst usage to know how much of a breaking change this would be and if we would want to support both (and if so, should it be opt-out of the newer formatting?) |
|
It would be great to have the option to opt for the colon syntax. Maybe it can be an opt-in option in the |
| LOGGER.warning("Invalid YAML in MyST directive options.") | ||
| return raw_content | ||
| if parsed: | ||
| formatted += "\n".join([f":{k}: {v}" for k, v in parsed.items()]) + "\n\n" |
There was a problem hiding this comment.
Isn't the problem here, that yaml.load from above will parse valueless options as {"hidden": None}, and then the emitted f-string would become: :hidden: None? That would sort of defeat the purpose.
| for name, plugin in mdformat.plugins.PARSER_EXTENSIONS.items() | ||
| if plugin in active_plugins | ||
| ] | ||
| formatted += mdformat.text( |
There was a problem hiding this comment.
Directive bodies are not always CommonMark. toctree entries use title <target> syntax; I think, running them through mdformat.text escapes the < to \<. This would rewrite every entry in a toctree, and probably break them.
| def format_directive_content(raw_content: str, context) -> str: | ||
| unformatted_yaml, content = parse_opts_and_content(raw_content) | ||
| formatted = "" | ||
| if unformatted_yaml is not None: |
There was a problem hiding this comment.
Doesn't parse_opts_and_content return None if the directive has no options but does have content, i.e. line 156? In that case, directives without options but with a body would lose their entire body.
| LOGGER.warning("Invalid YAML in MyST directive options.") | ||
| return raw_content | ||
| if parsed: | ||
| formatted += "\n".join([f":{k}: {v}" for k, v in parsed.items()]) + "\n\n" |
There was a problem hiding this comment.
I believe this line has another problem. If you have a code-cell
```{code-cell} python
---
tags: [hide-input, remove-output]
label: my-cell
---
print("hello")
the formatting works fine for scalars, but if there is a list it yields "['hide-input', 'remove-output']" but what it should look like is "('hide-input', 'remove-output')".
So this line needs to differentiate between:
- flags (just return bare
:key:) - scalars
- lists (comma separated lists in round brackets)
3 participants
Instead, move to the more modern recommendation (per the MyST docs)
of using the "colon" syntax for directive options.
I understand this PR might be much more controversial than #48 but I put it "on top" to be more convenient for what I believe to be the "typical" mdformat_myst user---someone who wants to use large the latest and greatest MyST conventions and just wants a formatter that works out-of-the-box.
Happy to squash/split, or otherwise modify commits as necessary! For example, if just this change might go in independentlyof #48, I can easily split off f5a3bd5
Fixes #47