Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

‼️ BREAKING: Remove dollarmath from default myst_enable_extensions #505

Merged
merged 3 commits into from Jan 19, 2022

Conversation

chrisjsewell
Copy link
Member

@chrisjsewell chrisjsewell commented Jan 14, 2022

Also, the tex2jax_ignore mathjax_ignore classes are only added to the document if the dollarmath extension is enabled.
Similarly, the mathjax configuration is only overriden if the dollarmath extension is enabled.

References

Also, the `tex2jax_ignore mathjax_ignore` classes are only added to the document if the `dollarmath` extension is enabled.
Similarly, the mathjax configuration is only overriden if the `dollarmath` extension is enabled.
@codecov
Copy link

codecov bot commented Jan 14, 2022

Codecov Report

Merging #505 (2582e97) into master (ad6e39d) will increase coverage by 0.18%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #505      +/-   ##
==========================================
+ Coverage   89.88%   90.06%   +0.18%     
==========================================
  Files          16       16              
  Lines        2095     2103       +8     
==========================================
+ Hits         1883     1894      +11     
+ Misses        212      209       -3     
Flag Coverage Δ
pytests 90.06% <100.00%> (+0.18%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
myst_parser/main.py 90.97% <ø> (+2.25%) ⬆️
myst_parser/sphinx_renderer.py 93.14% <ø> (ø)
myst_parser/docutils_renderer.py 91.78% <100.00%> (+0.06%) ⬆️
myst_parser/mathjax.py 95.83% <100.00%> (+0.18%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad6e39d...2582e97. Read the comment docs.

@chrisjsewell
Copy link
Member Author

I think we already agreed this makes sense @choldgraf, @mmcky
(and obviously jupyter-book sets this by default, so it will not affect that)

Copy link
Member

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems reasonable to me - if I recall, the major discussions were around how supporting dollar math creates a bunch of headaches for parsers and occasionally unexpected behavior in the results?

A couple of quick questions:

  • I guess this means that we are changing the "core MyST spec" since we are removing a syntax that was on by default. That's probably fine now since MyST hasn't hit an official "stable" spec yet, but I just wanted to confirm that you all agree that this is what we are doing here. (in the future, this is the kind of thing that an enhancement proposal process might be helpful for)
  • Does this mean that the recommended way for MyST Markdown to parse math is via a {math} directive? If so, perhaps that directive should be part of the "core" MyST spec, or alternatively we could decide that MyST doesn't treat mathematics in a special way at all in its core spec (not in this PR, just a note for the future)?
  • Did we have any of these discussions in a public space that we could cross reference? I can take a look when I have a moment next week but just wanted to mention it in case others know of issues/discussions/etc off-hand

@mmcky
Copy link
Member

mmcky commented Jan 17, 2022

@chrisjsewell @choldgraf -- sorry for my slow response, was on the road yesterday for 13 hours so offline. This is a really useful syntax for a lot of authors. I don't mind if it get's removed from myst-parser for stability reasons. However (as you say) I would recommend we leave it enabled it by default for jupyter-book. 👍

@chrisjsewell chrisjsewell linked an issue Jan 18, 2022 that may be closed by this pull request
@chrisjsewell
Copy link
Member Author

chrisjsewell commented Jan 18, 2022

Did we have any of these discussions in a public space that we could cross reference?

Yes this also was already discussed in #399, and you essentially already made part of this change in #385

I guess this means that we are changing the "core MyST spec"

dollarmath had already been moved to the optional syntax docs page in #385, so this is essentially already the case.

Does this mean that the recommended way for MyST Markdown to parse math is via a {math} directive?

This, in part, depends on what level of compatibility you want with "native" Jupyter Notebook (HTML) interfaces.
{math} obviously does not "degrade" as gracefully as using $ math, in this context.

It is kind of annoying that both sphinx and jupyter load mathjax by default, whose default configuration then "aggressively" converts all $ in the HTML to math.
This kind of gives the false impression that dollarmath is being directly supported by MyST, even if not enabled, and obviously the mathjax approach does not support non-HTML output formats
(up-front $ identification in the Markdown parser, is most definitely the better solution, which is what the dollarmath extension does)

I would recommend we leave it enabled it by default for jupyter-book

It does not affect jupyter-book, which "hard-codes" the default MyST extensions enabled

Copy link
Member

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍

A few quick responses:

Yes this also was already discussed in #399, and you essentially already made part of this change in #385

Thanks for digging up those links. I've updated the top comment with refs to those issues - just wanted to follow best practices to link back to conversations about stuff.

up-front $ identification in the Markdown parser, is most definitely the better solution, which is what the dollarmath extension does

If Jupyter moved away from marked.js and towards markdown-it, I wonder if this could be handled more gracefully by using a dollarmath extension in markdown-it, that would then render the right syntax for MathJax to process 🤔

@chrisjsewell
Copy link
Member Author

chrisjsewell commented Jan 19, 2022

Thanks for digging up those links. I've updated the top comment with refs to those issues - just wanted to follow best practices to link back to conversations about stuff.

cheers

If Jupyter moved away from marked.js and towards markdown-it, I wonder if this could be handled more gracefully by using a dollarmath extension in markdown-it, that would then render the right syntax for MathJax to process 🤔

Yep exactly; the "problem" at present, it that MathJax is acting as both a parser and a renderer,
Ideally, all the parsing (i.e. syntax tokenization) should be performed by the Markdown parser, then it just utliises mathjax/katex to turn the LaTeX into HTML.

For example, this is what https://github.com/executablebooks/markdown-it-dollarmath does

On the "python side", even more ideal, would be that you had a python implementation of mathjax/katex. Then the static HTML you generate would already have the math in HTML, and you wouldn't need to be running "dynamic" JavaScript.
This is essentially what happens with code blocks, that get converted to HTML (or whatever output format you need) by https://pygments.org/, during the sphinx/docutils build

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optional, but on by default syntax
3 participants