🐛 FIX: Parsing directive content, when body followed by arguments #28
Conversation
| if ( | ||
| !directive.required_arguments && | ||
| !directive.optional_arguments && | ||
| !Object.keys(options).length | ||
| ) { | ||
| if (!directive.required_arguments && !directive.optional_arguments) { |
There was a problem hiding this comment.
This is the change, all other changes are around the tests.
|
Hey @rowanc1 there is an issue, but not the one you think 😬 Firstly, with restructuredtext .. seealso:: A title
:class: tip
A body
.. seealso:: A title
A bodygives you:
with myst-parser ```{seealso} A title
:class: tip
A body
```
```{seealso} A title
A body
```you get
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 |
|
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:
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 |
| `see also` admonition with class, bump title new paragraph | ||
| . | ||
| ```{seealso} Not a title | ||
| :class: tip | ||
|
|
||
| See other things here! | ||
| ``` | ||
| . | ||
| <aside class="tip admonition seealso"> | ||
| <header class="admonition-title">See Also</header> | ||
| <p>Not a title</p> | ||
| <p>See other things here!</p> | ||
| </aside> | ||
| . |
There was a problem hiding this comment.
This is the test shown in the PR conversation that is passing!
| `see also` admonition with class, bump title | ||
| . | ||
| ```{seealso} Not a title | ||
| :class: tip | ||
| See other things here! | ||
| ``` | ||
| . | ||
| <aside class="tip admonition seealso"> | ||
| <header class="admonition-title">See Also</header> | ||
| <p>Not a title | ||
| See other things here!</p> | ||
| </aside> |
There was a problem hiding this comment.
This is the other relevant test case.
| ```{admonition} This is a title | ||
| :class: tip | ||
| An example of a `tip` with a custom _title_. | ||
| ``` | ||
| . | ||
| <aside class="tip admonition"> | ||
| <header class="admonition-title">This is a title</header> | ||
| <p>An example of a <code>tip</code> with a custom <em>title</em>.</p> | ||
| </aside> |
There was a problem hiding this comment.
All of these now put the custom class first, as is done in RST. I will make a note of that in the docs I am writing about this.
Ok cheers, so is this unrelated code something present in this repo, or some upstream thing? |
|
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! |
|
Checked in executablebooks/MyST-Parser#520, that this change does not break any tests over there 👍 |
chrisjsewell
changed the title
chrisjsewell
changed the title
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:
