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

switch from markdown2 to mistletoe #110

Merged
merged 6 commits into from
Feb 16, 2021
Merged

switch from markdown2 to mistletoe #110

merged 6 commits into from
Feb 16, 2021

Conversation

bryangingechen
Copy link
Collaborator

@bryangingechen bryangingechen commented Feb 14, 2021

markdown2 has a lot of trouble with latex+mathjax, see e.g. #10, #62 and various issues on Zulip. mistletoe is what leanprover-community.github.io uses and it seems to do better with these examples.

As part of this effort, I copied some of the implementation of Markdown + LaTeX from StackExchange's code math.stackexchange and mathoverflow. I believe #10 and #62 are now fixed, though some workarounds that were committed are now broken, e.g. the module doc string right before formal_multilinear_series.change_origin in analysis.analytic.basic. Compare mathlib_docs to mathlib_docs_demo.

For a bunch of LaTeX examples (including ones copied from the issues above), see the new test page, latex.html.

Since I was making a bunch of changes to the markdown rendering anyways, I went ahead and factored it all out into a separate file.

Some smaller changes:


I plan to follow this up with a PR to our naming conventions with some advice on LaTeX + Markdown, now that things should be more consistent.

@bryangingechen
Copy link
Collaborator Author

bryangingechen commented Feb 14, 2021

I'm going to try to modify the markdown processor so that it's as close to the behavior on https://math.stackexchange.com and https://mathoverflow.net as possible, since (in my opinion) those sites are the most significant places on the web right now where users frequently input LaTeX embedded in markdown.

This is likely to break some of the LaTeX we currently have, but I think the consistency will be worth it.

edit: After doing some more digging and experimentation, I'm not going to try to reproduce the stackexchange implementation or anything like that. I think we can get something that will agree on valid input by preventing the markdown processor from processing anything that MathJax would try to turn into math on a plain HTML document. With our current settings (which match those of stackexchange), these should just be strings surrounded by pairs of $, pairs of $$ or \begin{*} ... \end{*} that don't contain double linebreaks (since that puts them in different paragraphs).

References:

edit 2: Welp, the naïve approach I had in mind of just using regex to protect the math failed, so I ended up just porting some of the code used by StackExchange to Python.

@robertylewis
Copy link
Member

@bryangingechen thank you for taking this on! This change is definitely overdue.

I'm happy to merge this as soon as there's no obvious breakage worse than what it fixes. I see you marked this as a draft, is there more you're planning to change?

@gebner
Copy link
Member

gebner commented Feb 15, 2021

Ok, let's try this out!

#deploy 🐙

@robertylewis
Copy link
Member

@gebner
Copy link
Member

gebner commented Feb 15, 2021

Ok, the trigger seems to work! I've canceled the workflow, because I realized that we still need to merge master here in order to get the new deploy_docs.sh

@gebner
Copy link
Member

gebner commented Feb 15, 2021

#deploy

@robertylewis
Copy link
Member

Okay, this failed because leanprover-community-bot didn't accept the invitation to the demo repo. Trying again:

#deploy

@github-actions
Copy link

This PR has been successfully deployed at http://leanprover-community.github.io/mathlib_docs_demo!

@bryangingechen
Copy link
Collaborator Author

Oh, this new test workflow is cool!

I'm happy to merge this as soon as there's no obvious breakage worse than what it fixes. I see you marked this as a draft, is there more you're planning to change?

Yeah, I still haven't finished the changes described in my second comment above. The good news is I think I've already managed to fix #10. I'll try to clean things up and push a commit with my progress tonight.

@gebner
Copy link
Member

gebner commented Feb 15, 2021

#deploy and fix the $4

@github-actions
Copy link

This PR has been successfully deployed at http://leanprover-community.github.io/mathlib_docs_demo!

@bryangingechen
Copy link
Collaborator Author

#deploy 🙏

@github-actions
Copy link

This PR has been successfully deployed at http://leanprover-community.github.io/mathlib_docs_demo!

@bryangingechen bryangingechen deleted the mistletoe branch February 16, 2021 15:58
Comment on lines +53 to +60
ds_no_math, math = remove_math(ds, '$')
# We have to run `mathjax_editing.replace_math` on the text in code
# blocks before passing it to Pygments (see `render_block_code`),
# otherwise `replace_math` will be confused by the added syntax
# highlighting `<span>`s and won't be able to splice in those blocks.
self.math = math
html = self.render(Document(ds_no_math))
return replace_math(html, self.math)
Copy link
Member

Choose a reason for hiding this comment

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

I don't quite follow why this is necessary - does mistletoe.latex_token.Math not work here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I posted a reply here. But let's see how #116 turns out!

bors bot pushed a commit to leanprover-community/mathlib that referenced this pull request Feb 16, 2021
With leanprover-community/doc-gen#110, these should no longer be needed.





Co-authored-by: Rob Lewis <rob.y.lewis@gmail.com>
b-mehta pushed a commit to leanprover-community/mathlib that referenced this pull request Apr 2, 2021
With leanprover-community/doc-gen#110, these should no longer be needed.





Co-authored-by: Rob Lewis <rob.y.lewis@gmail.com>
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.

None yet

4 participants