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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悰 Fix tables overriding setext headings #27

Merged
merged 2 commits into from
Aug 3, 2023

Conversation

chrisjsewell
Copy link
Member

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
Copy link
Collaborator

rlidwka commented Jul 6, 2023

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
Copy link
Collaborator

rlidwka commented Jul 6, 2023

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
Copy link
Member Author

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
Copy link
Collaborator

rlidwka commented Aug 3, 2023

(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 rlidwka merged commit d7a6dc4 into markdown-it-rust:master Aug 3, 2023
2 checks passed
@rlidwka
Copy link
Collaborator

rlidwka commented Aug 3, 2023

Merged and released as 0.6.0, thanks!

@chrisjsewell
Copy link
Member Author

Thanks!

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

2 participants