markdown-it-rust / markdown-it

markdown-it js library rewritten in rust
Other
79 stars 9 forks source link

🐛 Fix tables overriding setext headings #27

Closed chrisjsewell closed 1 year ago

chrisjsewell commented 1 year ago

This fixes a bug in the port of table parsing, where effectively this line was omitted: https://github.com/markdown-it/markdown-it/blob/2b6cac25823af011ff3bc7628bc9b06e483c5a08/lib/rules_block/table.js#L123

rlidwka commented 1 year ago

Bug confirmed.

Note that markdown-it and cmark-gfm handle things differently.

This is a table in markdown-it, but not in cmark-gfm:

|foo
---

This is a table in cmark-gfm, but not in markdown-it:

foo
|---

I'm not sure what to do with that.

rlidwka commented 1 year ago

May I suggest to put tests in a different place? Everything in fixtures/markdown-it is copied from markdown-it.js as is. If we start adding custom tests there, might have difficulties syncing them later.

You can add tests in tests/extras.rs.

Or create a new unit test in src/plugins/extra/tables.rs, only enable that rule and test it directly with something like this:

#[test]
fn require_pipe_in_header_row() {
    let md = &mut crate::MarkdownIt::new();
    crate::plugins::extra::tables::add(md);
    let html = md.parse("foo\n---\nbar").render();
    assert_eq!(html.trim(), "foo\n---\nbar");
    let html = md.parse("|foo\n---\nbar").render();
    html.trim().starts_with("<table");
}
chrisjsewell commented 1 year ago

Hey @rlidwka I moved the test to src/plugins/extra/tables.rs as you suggested.

I also changed the logic to check that the alignment row contains at least one : or |, since this appears to be more in alignment with cmark-gfm, as you noted

(I was also trying to make sense of https://github.com/kivikakk/comrak/blob/72b5209405e6177ff04577013daa7febd70fe615/src/scanners.rs#L21388, but the logic was not entirely clear)

rlidwka commented 1 year ago

(I was also trying to make sense of https://github.com/kivikakk/comrak/blob/72b5209405e6177ff04577013daa7febd70fe615/src/scanners.rs#L21388, but the logic was not entirely clear)

According to a comment on top, that file is "Generated by re2c 3.0". What you are seeing is a state-machine. If you want to understand the logic, it's probably better to look what regexp it was generated from.

rlidwka commented 1 year ago

Merged and released as 0.6.0, thanks!

chrisjsewell commented 1 year ago

Thanks!