Repository navigation
Issue with urls inside markdown tables #127
Description
Hi! I'm trying to upgrade the turing way book to the new jupyter-book beta version and found a bug when I want to link a url inside a table. I'm kind of new in the world of sphinx, commonmark and parsers, so this might be a bug I need to report to sphinx instead of here. I'm sorry if that is the case!
To give a reproducible example of the error, if I want to build jupyter-book using a single markdown file containing the following:
| Prerequisite | Importance | Notes |
|---|---|---|
| Version Control | Very Important | |
| Reproducible Environments | Very Important | [Binder](https://mybinder.org/) |
(rendered markdown version below)
| Prerequisite | Importance | Notes |
|---|---|---|
| Version Control | Very Important | |
| Reproducible Environments | Very Important | Binder |
I get the following error:
Exception occurred:
File "/Users/martina.gonzales/anaconda3/envs/executable-book/lib/python3.7/site-packages/sphinx/writers/html5.py", line 211, in visit_reference
assert len(node) == 1 and isinstance(node[0], nodes.image)
AssertionError
But if I link it outside the table:
[Binder](https://mybinder.org/)
there is no issue.
One possible reason for the issue, but I might be totally wrong in this: Reading the sphinx error message in context, it seems that the writer in sphinx is expecting the node parent of the reference to be a TextElement. But when I used the MyST API to manually convert these texts to docutils I get that the parent of the reference is <entry>, which I don't think is of text kind. In the isolated reference case (when is not inside the table), the parent node is <paragraph>.
I'm using:
- sphinx [2.4.4]
- myst-parser [0.8.0]
- python [3.7.7]
cc @choldgraf
Activity
choldgraf commentedon Apr 14, 2020
Hmm, I can confirm this breaks on my machine too, and with this minimal example:
I think that the fix would be somewhere here: https://github.com/ExecutableBookProject/MyST-Parser/blob/f28529919f261178ec7add8dcd61fbdb594123f1/myst_parser/docutils_renderer.py#L531 but maybe @chrisjsewell has an idea?
chrisjsewell commentedon Apr 14, 2020
Hey @martinagvilas thanks for the report.
Well a little lower down: https://github.com/ExecutableBookProject/MyST-Parser/blob/f28529919f261178ec7add8dcd61fbdb594123f1/myst_parser/docutils_renderer.py#L534
This is where children of the row are added as children of the
entrynode.Firstly I would look at: https://github.com/ExecutableBookProject/MyST-Parser/blob/master/tests/test_renderers/fixtures/tables.md (in raw format), these are the tests defining exactly how the markdown is converted to docutils AST. We would want to add additional test(s) here to clarify the current/desired conversion.
Then I would look here: https://github.com/docutils-mirror/docutils/blob/master/test/test_parsers/test_rst/test_tables.py, at the equivalent behaviour for the rST parser.
As you mention, it may be that for your case the link is not being wrapped in a paragraph. Of the top of my head, it would also be good to check if this is always the case, or is it because you only have the single link in the cell, e.g. what would be the outcome for these:
choldgraf commentedon Apr 14, 2020
Quick update:
and
do not work
martinagvilas commentedon Apr 14, 2020
Indeed, inspecting the output of
to_docutils()ofshows no
paragraphwrapper inentry:as outputs in these tests expect
martinagvilas commentedon Apr 14, 2020
What about changing this to something like this:
... to wrap the paragraph as well.
I tried it and it seems to render ok.
choldgraf commentedon Apr 14, 2020
@martinagvilas wow thanks for prototyping that fix! Glad that you were able to orient yourself to the codebase quickly. That looks reasonable to me, but @chrisjsewell would know better than I do. Are there other places in the code where we just "wrap" an element in another element?
chrisjsewell commentedon Apr 14, 2020
Well its easy, when its such a well written code base 😆
Yeh that's pretty much it 👍 , I should be able to add it in the next day.
Or feel free to make a PR, but make sure the requisite tests are added, and we make want to also check that things like https://myst-nb.readthedocs.io/en/latest/use/glue.html#pasting-into-tables are still work before merging
choldgraf commentedon Apr 14, 2020
Indeed - I must admit I also made a mental note that this must be a well-written codebase :-)
martinagvilas commentedon Apr 15, 2020
It is a great codebase, I agree 🚀
I can try to submit a PR with some tests if you haven't started working on that already.
I checked the glue features (amazingly awesome 😄) and it seems to be rendering fine with the changes:
The plots do look smaller than here but I tried rendering the same tables without the
<paragraph>wrapper changes and the size is the same, so I think it is a separate issue.Wrap table cell content in paragraph node (#128)
Try to deal with weird JB URL issue
ncalexan commentedon Feb 21, 2024
We also are having this exact issue: https://bugzilla.mozilla.org/show_bug.cgi?id=1828390.
choldgraf commentedon Feb 21, 2024
@ncalexan I thought this was resolved in
#128
Can you confirm you're on the latest Myst parser?
ncalexan commentedon Feb 21, 2024
We're on
(from https://searchfox.org/mozilla-central/rev/d405168c4d3c0fb900a7354ae17bb34e939af996/python/sites/docs.txt#7) and I see from https://pypi.org/project/myst-parser/ that the latest is 2.0.0, so we're well behind.
Thank you so much for suggesting the update, I'll see if we can get this fixed!