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 multiple depth levels list parsing (second attempt!) #71

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

mflint
Copy link

@mflint mflint commented Mar 12, 2022

This is another attempt to merge PR #53, created by @zntfdr.

Their original PR received no love, and I'm hoping that @JohnSundell might see this second attempt :)

Why am I making a new PR for somebody else's code?

  • I believe this PR is a useful contribution to Ink
  • I've been using a fork of Ink - with these changes - for a while with no problems
  • @zntfdr added new unit tests to test the new code, and didn't change any existing tests - so I'd be confident that there are no regressions

Thank-you, John, for taking the time to consider this PR! 🤞

@mflint mflint reopened this Apr 7, 2023
@mflint
Copy link
Author

mflint commented Apr 7, 2023

(Messed up my rebase, and accidentally closed the PR 🤦‍♂️ . Should be OK now...)

@magnuskahr
Copy link

Lets please have this merged 🙏🏻

@kudit
Copy link

kudit commented Oct 20, 2023

Sorry to leave this here but I couldn't find a way to just report an issue. I noticed that with the list parser, you can create unordered lists like this:

Header:
- One
- Two
- Three

And it works fine. However, ordered lists do not work if there isn't a blank line after the header.

Header:
1. One
2. Two
3. Three

Doesn't work but

Header:

1. One
2. Two
3. Three

Does work.

Also, I noticed that headers don't work if there's not a space after the hash marks which I can't tell if that's part of the spec or not.

@afoeder
Copy link

afoeder commented Nov 9, 2023

@JohnSundell Please merge this, it has tests and looks good.

Copy link

@afoeder afoeder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@mkassoff
Copy link

This seems like it is missing something from #53, namely the lines:

                if !reader.didReachEnd {
                    switch (indentationLength, reader.currentCharacter) {
                    case (0, _): continue
                    case (_, \.isWhitespace): continue
                    case (_, _): return list
                    }
                }

Thus, for example, the following Markdown is incorrectly parsed:

- Fruit
    - Apple
- Dairy

Which is parsed as:

• Fruit
    • Apple
    • Dairy 

@afoeder
Copy link

afoeder commented Nov 11, 2023

I have to double-check then because I added a test that checks for exactly this (I‘m here because this bug troubles me) and the test worked with this patch only. But maybe I got lost somewhere.

@afoeder
Copy link

afoeder commented Nov 11, 2023

@mkassoff : I cannot confirm what you've said, the tests that are added "should" cover your cases since there are more complicated nestings covered which are parsed correctly. To double-check, I've 1:1 checked your example as a test case:

let html = MarkdownParser().html(from: """
- Fruit
    - Apple
- Dairy
""")

let expectedComponents: [String] = [
    "<ul>",
        "<li>Fruit",
            "<ul>",
                "<li>Apple</li>",
            "</ul>",
        "</li>",
        "<li>Dairy</li>",
    "</ul>"
]

XCTAssertEqual(html, expectedComponents.joined())

and it runs just fine as well on this PR.

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.

6 participants