wooorm / markdown-rs

CommonMark compliant markdown parser in Rust with ASTs and extensions
https://docs.rs/markdown/1.0.0-alpha.18/markdown/
MIT License
836 stars 41 forks source link

Mark mdast lists as spread if any children are spread #103

Closed begleynk closed 5 months ago

begleynk commented 5 months ago

Firstly, thanks for creating a great library! πŸš€

I ran into what I think is a bug in how List nodes are marked as spread. The docs say this:

One or more of its children are separated with a blank line from its siblings (when true), or not (when false).

In my testing this didn't seem to be the case. I'd see a spread ListItem, but the parent List was still marked as not spread.

I looked into the code, added a failing test, and it seemed like list_loose was being called incorrectly (at least based on the docstring). Fixing this made the tests pass. I also fixed one existing test that seemed to be asserting incorrectly.

Hope that makes sense! If I'm missing something here, just let me know.

begleynk commented 5 months ago

Oops accidentally closed.

Not sure what I should do about those clippy errors. Should those be addressed here?

wooorm commented 5 months ago

From what you say, both the docs and the code are correct! Your explanation has the problem: whether the list itself is spread is about what’s between the items. Not about the items.

wooorm commented 5 months ago

Loose is a different concept; Spread on both the list and the items needs to be considered to know if a list is loose or not.

begleynk commented 5 months ago

Aha I see! It didn't occur to me that the List itself could be spread, but that makes a lot of sense of course. For context, I'm writing an mdast -> HTML renderer.

I guess the important point is that this:

- foo

- bar

- baz

renders to

<ul>
<li>
<p>foo</p>
</li>
<li>
<p>bar</p>
</li>
<li>
<p>baz</p>
</li>
</ul>

instead of

<ul>
<li>foo</li>
<li>bar</li>
<li>baz</li>
</ul>

Even though the ListItem nodes themselves are not spread.

Thanks for the explanation. I think this PR can be closed πŸ‘

wooorm commented 5 months ago

I recommend to look at the existing JavaScript ecosystem, which already has all you are looking for. See mdast-util-to-hast and hast-util-to-html!