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

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

Conversation

begleynk
Copy link
Contributor

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 begleynk closed this Jan 29, 2024
@begleynk begleynk reopened this Jan 29, 2024
@begleynk
Copy link
Contributor Author

Oops accidentally closed.

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

@wooorm
Copy link
Owner

wooorm commented Jan 29, 2024

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

wooorm commented Jan 29, 2024

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

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

wooorm commented Jan 29, 2024

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!

@wooorm wooorm closed this Jan 29, 2024
@begleynk begleynk deleted the mark-list-as-spread-if-any-children-spread branch January 30, 2024 07:25
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.

2 participants