👌 IMPROVE: Alternative proposal for allowing colon fences#48
👌 IMPROVE: Alternative proposal for allowing colon fences#48brunobeltran wants to merge 2 commits intoexecutablebooks:masterfrom
Conversation
brunobeltran
force-pushed
the
bruno/colon-fence-for-upstream
branch
from
df6eb7b to
4b3ff3a
Compare
|
@KyleKing are you still the right person to review this? Sorry for the inbox-blast if not (and please let me know if there's a better comms channel than just GitHub comments for this)! |
|
Great; thank you @brunobeltran for taking over ;-) |
KyleKing
left a comment
KyleKing
left a comment
There was a problem hiding this comment.
Some minor comments and overall looks like this should work with some caveats on performance. Thanks!
| :::{tip} Nested tip in list item | ||
| Tip content inside a list item. | ||
| ::: | ||
| . |
There was a problem hiding this comment.
If not already added, can you make sure that other edge cases are tested? For example: with an empty directive, with arguments, containing a code block and deeper nesting, and maybe one to test that regular content with ::: is handled
Additionally, are any tests from #36 not covered here?
| return text | ||
|
|
||
|
|
||
| CHANGES_AST = True |
There was a problem hiding this comment.
Is this strictly necessary? Turning this on can introduce a lot of risk -- I'm assuming this is required to deal with the recursion?
| plugin = mdformat.plugins.PARSER_EXTENSIONS[plugin_name] | ||
| if plugin not in mdit.options["parser_extension"]: | ||
| mdit.options["parser_extension"].append(plugin) | ||
| plugin.update_mdit(mdit) |
| formatted += mdformat.text( | ||
| content, options=context.options, extensions=extension_names | ||
| ) |
There was a problem hiding this comment.
I'm a little worried about this the performance here and would prefer to have native support like regex parsing (proposed in #36), but directive content should be small and is something we could revisit
There was a problem hiding this comment.
The bigger issue is that I don't think any other plugin uses mdformat recursively like this, so it could lead to unexpected behavior
3 participants
An alternative proposal to #36 (fixes #13, thanks @nthiery for your pass at this, which inspired me to give it a shot).
Avoids the need for an explicit list of "container_names" (which doesn't seem like an appropriate thing for mdformat_myst to manage) by instead recursively calling mdformat's API directly. I'm not sure if there are any negative consequences to this, as I'm not very knowledgable on the mdformat codebase, but it doesn't seem to lead to a performance regression on $DAY_JOB's monorepo, at least.
Note that this PR also proposes a very noticeably different set of semantics for when to add padding newlines, namely: I only add padding newlines around child colon fences, so "regular" (e.g. paragraph) content does not get additional padding, maintaining more similarity with the current behavior.