executablebooks / markdown-it-docutils

A markdown-it plugin for implementing docutils style roles/directives.
https://executablebooks.github.io/markdown-it-docutils/
MIT License
12 stars 8 forks source link

🐛 FIX: Parsing directive content, when body followed by arguments #28

Closed rowanc1 closed 2 years ago

rowanc1 commented 2 years ago

There is a bug that the content did not get bumped if there were options.

I have rearranged the admonition tests to add some more coverage for this.

This is the bug: broken-admonition

chrisjsewell commented 2 years ago

Hey @rowanc1 there is an issue, but not the one you think 😬

Firstly, seealso has 0 required/optional arguments, i.e. the title here is not a title, it's actually body text 😕


with restructuredtext

.. seealso:: A title
    :class: tip

    A body

.. seealso:: A title

    A body

gives you:

image


with myst-parser

```{seealso} A title
:class: tip

A body

A body


you get

![image](https://user-images.githubusercontent.com/2997570/153968639-196b05da-92c2-489d-8928-946352fce3b4.png)

---

So https://github.com/executablebooks/MyST-Parser/blob/e477a75940b7a943ca834a98cbea8bcb8d824a6c/myst_parser/parse_directives.py#L52, is "mistakenly" omitting the first line, wheras the code here is mistaking it for a title

Although yeh, as we've discussed before, the "variability of the directive structure does mean that syntax can be terser, but its not ideal for the parser
rowanc1 commented 2 years ago

The GIF that I included was a poor choice, there was some other code that was marking the first paragraph as the title that is unrelated. The test does show the appropriate change, although there is additional work that needs to bring the class to the front to make that the first choice in styling.

The code change that is in this PR produces the following in mystjs:

image

I think we do have the choice in MyST to be stricter/more sensible than RST here if we want (I think we should!). We could introduce warnings when people put in arguments that is interpreted as inline content, and eventually deprecate that syntax in MyST. Regardless, I think we should make the behaviour consistent with RST for now, and this PR does do the trick!

I will introduce another test case to cover the tip class being at the front.

chrisjsewell commented 2 years ago

there was some other code that was marking the first paragraph as the title that is unrelated

Ok cheers, so is this unrelated code something present in this repo, or some upstream thing?

rowanc1 commented 2 years ago

The header flicker had to do with the mystjs repo and the mdast tree (since fixed), not related at all to docutils or this change. The tests I highlighted in the review in this repo and the exported html from the myst are the place to look to verify this change is correct!

chrisjsewell commented 2 years ago

Checked in https://github.com/executablebooks/MyST-Parser/pull/520, that this change does not break any tests over there 👍