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

Add math support #366

Merged
merged 11 commits into from
Mar 29, 2024
Merged

Add math support #366

merged 11 commits into from
Mar 29, 2024

Conversation

digitalmoksha
Copy link
Collaborator

@digitalmoksha digitalmoksha commented Feb 18, 2024

See #366 (comment) for updated details.


This will add support for math syntax, both the dollar math and the newer GitLab/GFM syntax. Relates to #23

Display math is mostly working, $$ and ```math syntax. Inline not at all yet.

A couple notes:

  • there is no inline math support yet

  • Currently the $$ syntax follows the rules of code blocks. Meaning that

    This is a paragraph
    $$
    1 + 2
    $$
    and this is a separate paragraph.
    

    generates a paragraph followed by a block and then another paragraph. I've seen some implementations where it is all in one paragraph. I'm torn which way it should be. At the moment this is the easiest.

  • The $$ math blocks are generated the same way that ```math blocks are generated, as a <pre><code> block. This keeps the code and the output consistent. It also means that if math rendering is not enabled on a site, the math will display as a code block, rather than a bunch of run-together text, without any extra styling needed.

  • I'm using data attributes rather than classes. So it's <pre data-math-style="display"><code>... rather than <pre class="math"><code>....

I'm flexible on these decisions, this is just where it's at right now.

@digitalmoksha digitalmoksha mentioned this pull request Feb 18, 2024
@digitalmoksha digitalmoksha force-pushed the bw-add-math branch 2 times, most recently from 7606d03 to 2f6d586 Compare March 22, 2024 19:15
@digitalmoksha
Copy link
Collaborator Author

digitalmoksha commented Mar 23, 2024

@kivikakk I think I have this finished 🤞

It's similar to pandoc and commonmark-hs for dollar math, including using <span>. But it uses a data-math-style attribute to determine whether it's inline or display.

Code math (the GitLab syntax), uses <code> instead of <span>, which I think is correct given the markdown syntax. And it gives a way to tell the two syntaxes apart if the HTML needs to be converted back into markdown (which we do).

If you get some time, would you mind reviewing?

cargo run -- -e math-dollars enables dollar math.

  • Inline math

    Calc $x^2$ and $$y^2$$

    <p>Calc <span data-math-style="inline">x^2</span> and <span data-math-style="display">y^2</span></p>
  • block math

    $$
    z^2
    $$
    
    <p><span data-math-style="display">
    z^2
    </span></p>

cargo run -- -e math-code enables code/GitLab math.

  • Inline math

    Calc $`x^2`$

    <p>Calc <code data-math-style="inline">x^2</code></p>
  • block math

    ```math
    z^2
    ```
    
    <pre><code class="language-math" data-math-style="display">z^2
    </code></pre>

@kivikakk
Copy link
Owner

heya @digitalmoksha, thank you so much for this! i'm currently preparing for an imminent overseas move, but i'll try to squeeze this in in the next couple days :)

@digitalmoksha
Copy link
Collaborator Author

And good luck on your move! Exciting!

Copy link
Owner

@kivikakk kivikakk left a comment

Choose a reason for hiding this comment

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

I've read through everything closely with the exception of inlines.rs; will look at that more carefully tonight! I did a quick check that "stopping" on $ unilaterally in the parsing loop doesn't affect e.g. autolinks. I'll also do some fuzzing.

src/nodes.rs Show resolved Hide resolved
src/parser/math.rs Outdated Show resolved Hide resolved
src/tests/math.rs Outdated Show resolved Hide resolved
Copy link
Owner

@kivikakk kivikakk left a comment

Choose a reason for hiding this comment

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

never mind that, I'm having a flare up and will be out of action for a bit again! please use your best judgment and run fuzzing for a while, and merge when ready. ❤️

@digitalmoksha
Copy link
Collaborator Author

Oh I'm sorry to hear that. I hope you feel better.

Thank you for allowing me to move forward on this 🙇. I will definitely run fuzzing and take another look at the commented out tests.

@digitalmoksha digitalmoksha self-assigned this Mar 26, 2024
@kivikakk
Copy link
Owner

Just fwiw, this branch did hit a crash on cargo fuzz run all_options -j4 within a minute or two, so it'd be worth going through a few fuzz–fix cycles!

@kivikakk
Copy link
Owner

Minimised repro of the one I hit first:

> printf '$`$' | RUST_BACKTRACE=1 cargo run -- -e math-dollars -e math-code
    Finished dev [unoptimized + debuginfo] target(s) in 0.03s
     Running `target/debug/comrak -e math-dollars -e math-code`
thread 'main' panicked at src/parser/inlines.rs:726:42:
slice index starts at 2 but ends at 1
stack backtrace:
   0: rust_begin_unwind
             at /rustc/2f1bd0729b74787f55d4cbc7818cfd787cd43a99/library/std/src/panicking.rs:597:5
   1: core::panicking::panic_fmt
             at /rustc/2f1bd0729b74787f55d4cbc7818cfd787cd43a99/library/core/src/panicking.rs:72:14
   2: core::slice::index::slice_index_order_fail_rt
             at /rustc/2f1bd0729b74787f55d4cbc7818cfd787cd43a99/library/core/src/slice/index.rs:98:5
   3: core::slice::index::slice_index_order_fail
             at /rustc/2f1bd0729b74787f55d4cbc7818cfd787cd43a99/library/core/src/slice/index.rs:91:14
   4: <core::ops::range::Range<usize> as core::slice::index::SliceIndex<[T]>>::index
             at /rustc/2f1bd0729b74787f55d4cbc7818cfd787cd43a99/library/core/src/slice/index.rs:406:13
   5: core::slice::index::<impl core::ops::index::Index<I> for [T]>::index
             at /rustc/2f1bd0729b74787f55d4cbc7818cfd787cd43a99/library/core/src/slice/index.rs:18:9
   6: comrak::parser::inlines::Subject::handle_dollars
             at ./src/parser/inlines.rs:726:42
   7: comrak::parser::inlines::Subject::parse_inline
             at ./src/parser/inlines.rs:219:25
   8: comrak::parser::Parser::parse_inlines
             at ./src/parser/mod.rs:2014:15
   9: comrak::parser::Parser::process_inlines_node
             at ./src/parser/mod.rs:1994:17
  10: comrak::parser::Parser::process_inlines
             at ./src/parser/mod.rs:1988:9
  11: comrak::parser::Parser::finalize_document
             at ./src/parser/mod.rs:1827:9
  12: comrak::parser::Parser::finish
             at ./src/parser/mod.rs:1809:9
  13: comrak::parser::parse_document_with_broken_link_callback
             at ./src/parser/mod.rs:123:5
  14: comrak::parser::parse_document
             at ./src/parser/mod.rs:62:5
  15: comrak::main
             at ./src/main.rs:286:16
  16: core::ops::function::FnOnce::call_once
             at /rustc/2f1bd0729b74787f55d4cbc7818cfd787cd43a99/library/core/src/ops/function.rs:250:5

Inline parsing of $$ handles the block style
while providing better compatability
with other parsers.
@digitalmoksha
Copy link
Collaborator Author

@kivikakk I've been running fuzzing for about 5 hours without any problems, and will probably let it run overnight. I feel this is ready to go.

Let me know if you feel like giving it another once-over before I merge it.

@digitalmoksha digitalmoksha merged commit 05a2532 into kivikakk:main Mar 29, 2024
12 checks passed
@digitalmoksha
Copy link
Collaborator Author

I've been running fuzzing for about 15 hours without problems.

I'm going to go ahead and merge this.

@digitalmoksha
Copy link
Collaborator Author

@kivikakk before you become completely unavailable with your move, would you have time to cut a new release? I'm particularly hoping to get the escaped chars option released.

@kivikakk
Copy link
Owner

kivikakk commented Mar 29, 2024 via email

@kivikakk
Copy link
Owner

Release underway …

@kivikakk
Copy link
Owner

Done! https://crates.io/crates/comrak/0.22.0 / https://github.com/kivikakk/comrak/releases/tag/0.22.0

Thank you all. 😊

@gjtorikian
Copy link
Collaborator

BTW @kivikakk if it is at all helpful I can help cut releases too—I just don’t know how or whether I have the access 😅

@kivikakk
Copy link
Owner

@gjtorikian Thank you very much! Invite sent; bus factor halved! ✅

@gjtorikian
Copy link
Collaborator

Thanks for that! Do you just cargo publish locally or is there any automation for git tags and GitHub releases?

@kivikakk
Copy link
Owner

No automation, just RELEASE_CHECKLIST.md in the root! (It'd be lovely to automate but frankly it's never become worth doing.)

@digitalmoksha
Copy link
Collaborator Author

I updated the dingus to allow the enabling of math (and the enabling of most other options, though I'm still missing a couple)

@gjtorikian
Copy link
Collaborator

No automation, just RELEASE_CHECKLIST.md in the root! (It'd be lovely to automate but frankly it's never become worth doing.)

I am a lazy, lazy person, so I will see how much a robot can do for us. 😆

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.

3 participants