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

Improve list item parsing and indented code blocks parsing #88

Merged
merged 33 commits into from Aug 20, 2021

Conversation

LutSa
Copy link
Collaborator

@LutSa LutSa commented Jun 20, 2021

What's in this PR:

  1. solve the issue of Thematic breaks should take precedence over new bullet item parsing #59 by using the technique of Open/Closed Blocks
  2. change the UnorderedList type to a block container like BlockQuote
  3. associate indent information to unordered list item and allow some nest specs according to the indent (and solve unordered list parsing doesn't work with multilines item #81)
  4. solve some minimal failing test cases about list item parsing by the way
  5. fix minor failing test cases about “Indented code blocks” by allowing blank line between chunks

Update: How I parse Lists and List items

  1. parse the string to RawBlock type UnorderedListBlock/OrderedListBlock
    1. [Changes in RawBlock.elm ] Firstly, we need to change the RawBlock type UnorderedListBlock/OrderedListBlock to contain “List RawBlock” instead of “List UnparsedInlines” because list item is container block that can contain other blocks. We also need to contain more information in the type UnorderedListBlock/OrderedListBlock to support Open/Close Block method: tight/loose, intended, marker. Because we parse the string to RawBlock when the block is closed, we also need to contain unparsed string as an open block that can merge later unparsed string.
    2. [Changes in UnorderedList.elm and OrderedList.elm] Then, we need to change the parsers that parse string to some information that the RawBlock type UnorderedListBlock/OrderedListBlock need. Generally, I deleted the part where subsequent items are parsed because we only parse a single list item here and merge this list item by Open/Close Block method later. And more information (intended and marker) is gotten. And there are also some minor fixes in order to fit the spec. (I’m sorry that the code style in these two files are bad.)
    3. [Changes in RawBlock Parser unorderedListBlock and orderedListBlock in Parser.elm] Finally, the functions that convert the needed informations to RawBlock type UnorderedListBlock/OrderedListBlock also need to change.
  2. apply Open/Close Block method to the parsing of UnorderedListBlock/OrderedListBlock.
    1. I made some changes in completeOrMergeBlock and completeBlock. I just applied the similar way of Open/Close Block method as we always use to merge list type. Changes in completeOrMergeBlocks in Parser.elm (when the open block is UnorderedListBlock); Changes in completeOrMergeBlocks in Parser.elm (when the open block is OrderedListBlock); Changes in completeOrMergeBlocks in Parser.elm (when the open block is Blankline::OrderedListBlock or UnorderedListBlock); Changes in completeBlocks
    2. Also, I match the intended of the open block and unparsed string to support intended list item here.
  3. [Changes in Block.elm] Here, I just did some adjustments according to the type change (Inline -> Block in Block type UnorderedList/OrderedList; UnparsedInline -> RawBlock in RawBlock type UnorderedList/OrderedList)
  4. [Changes in Render.elm] Here, we only need to did some adjustment saccording to the type change (Inline -> Block in Block type UnorderedList/OrderedList. We also need to render according to tight/loose. Thanks to Jonas’ work, I completely use his code here.

LutSa and others added 22 commits June 4, 2021 20:02
…rawBlockParser when a container is completed/close)
… using the technique of Open/Closed Blocks and changing UnorderedList type to a block container
@dillonkearns
Copy link
Owner

dillonkearns commented Jul 19, 2021

It's nice that the actual renderer functions that users write won't actual need to change.
I discussed this design a while ago with @klaftertief and we could never fully decide on where the loose/tight list item rendering responsibility should be. Loose/tight handling be done at one of two levels:

  1. within the user's renderer functions, or
  2. in the code that calls the user's renderer functions

The implementation you have here uses approach (2). The logic lives outside the user's renderer functions. It's nice to see the full implementation, it makes it easier for me to understand which approach I like more. And seeing your implementation, my initial feeling is that I like this approach. It would seem very low-level if the user had to handle those details of loose/tight, and I imagine that most users would just find it frustrating, and wouldn't really find it useful to be able to customize how the render based on loose/tight, but instead would just want that detail taken care of in the implementation.

It looks like remark has a similar design. You can see in this example that they parse lists as spread: true for loose lists and spread: false for tight lists: https://astexplorer.net/#/gist/0a92bbf654aca4fdfb3f139254cf0bad/fe0a57596d677919245eca2b032c2a5ca892ec85.

Also for reference, here is the MDXJS syntax tree reference for lists: https://github.com/syntax-tree/mdast#list.

And since the user can map over items in the parsed Markdown Blocks, they also have the option to transform blocks and change whether a list is loose or tight, that gives at least some customizability there if it's needed.

@dillonkearns
Copy link
Owner

For future reference, I thought of a 3rd design idea for how to handle loose/tight lists.

  1. By default, automatically use the paragraph renderer to wrap loose paragraphs (as the current implementation does). Or, if a looseListWrapper is provided (Just value instead of Nothing), then use that to customize how to wrap loose lists.

I'm not sure there would be any value to approach (3) compared to the current approach, (2). I'm just including it here for future reference. If anyone can think of a use case where approach (3) would be beneficial, I would love to hear about it, though!

@LutSa
Copy link
Collaborator Author

LutSa commented Aug 18, 2021

I’ve let all test cases passed. But since I’ve changed the parser in UnorderedList.elm and OrderedList.elm from the parser of the list to the parser of the list item, the test cases may not be so meaningful as before. Please tell me if I can improve the test cases somehow.

@dillonkearns
Copy link
Owner

Excellent! I think it would be nice to have those unit tests exercising the full list parsing, so since that logic has moved up a level I made a commit to run the full parser on those test cases to keep them meaningful: d5d1e13.

I think that's a good balance that will keep giving us confidence about those corner cases with the new parsing, so I think the tests are in good shape now 👍

src/Markdown/Block.elm Outdated Show resolved Hide resolved
src/Markdown/ListItem.elm Show resolved Hide resolved
test-results/failing/New/adjacent_lists.md Outdated Show resolved Hide resolved
@dillonkearns dillonkearns merged commit 1c0ca65 into dillonkearns:master Aug 20, 2021
@dillonkearns
Copy link
Owner

It's all looking good to me now! Thanks so much for the hard work @LutSa! 🎉

@dillonkearns dillonkearns mentioned this pull request Aug 20, 2021
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