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

Add basic handling for nested lists #23

Merged
merged 3 commits into from
Apr 21, 2024

Conversation

RohanM
Copy link
Contributor

@RohanM RohanM commented Apr 19, 2024

https://app.bugsnag.com/the-conversation/tc/errors/6576cc832f734a0008ef8789?filters[error.status]=open&filters[event.since]=30d

We've seen a slow but continual stream of errors related to markdown rendering of nested lists. We'd like to offer at least a basic solution to this common use case. The output of this change isn't valid markdown, but represents a human-
readable result, which is at least a step forward.

Before:

NoMethodError: undefined method `strip' for ["* My list item"]

After:

* * My nested list item
1. 1. My nested list item

@RohanM
Copy link
Contributor Author

RohanM commented Apr 19, 2024

We've seen a slow but continual stream of errors related to markdown rendering of
nested lists. We'd like to offer at least a basic solution to this common use
case. The output of this change isn't valid markdown, but represents a human-
readable result, which is at least a step forward.
@RohanM RohanM force-pushed the 16814-upmark-add-basic-nested-list-handling branch from a6a7327 to 8f1d9b9 Compare April 19, 2024 00:48
@RohanM RohanM marked this pull request as ready for review April 19, 2024 00:49
expect(html).to convert_to("1. 1. List item")
end
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Checking out these new tests to the main branch will raise the following errors in the test suite, as expected:

╰─$ bundle exec rspec                                              

Randomized with seed 12685
..F................F..........................................................................

Failures:

  1) Upmark.convert nested unordered lists generates readable output
     Failure/Error: children = element[:children].map {|value| value.strip != "" ? value : nil }.compact
     
     NoMethodError:
       undefined method `strip' for ["* List item\n"]:Array
     # ./lib/upmark/transform/markdown.rb:51:in `block (2 levels) in <class:Markdown>'
     # ./lib/upmark/transform/markdown.rb:51:in `map'
     # ./lib/upmark/transform/markdown.rb:51:in `block in <class:Markdown>'
     # ./lib/upmark/transform_helpers.rb:22:in `block (2 levels) in element'
     # ./lib/upmark.rb:20:in `convert'
     # ./spec/acceptance/upmark_spec.rb:8:in `actual'
     # ./spec/acceptance/upmark_spec.rb:4:in `block (3 levels) in <top (required)>'
     # ./spec/acceptance/upmark_spec.rb:357:in `block (3 levels) in <top (required)>'

  2) Upmark.convert nested ordered lists generates readable output
     Failure/Error: children = element[:children].map {|value| value.strip != "" ? value : nil }.compact
     
     NoMethodError:
       undefined method `strip' for ["1. List item\n"]:Array
     # ./lib/upmark/transform/markdown.rb:56:in `block (2 levels) in <class:Markdown>'
     # ./lib/upmark/transform/markdown.rb:56:in `map'
     # ./lib/upmark/transform/markdown.rb:56:in `block in <class:Markdown>'
     # ./lib/upmark/transform_helpers.rb:22:in `block (2 levels) in element'
     # ./lib/upmark.rb:20:in `convert'
     # ./spec/acceptance/upmark_spec.rb:8:in `actual'
     # ./spec/acceptance/upmark_spec.rb:4:in `block (3 levels) in <top (required)>'
     # ./spec/acceptance/upmark_spec.rb:373:in `block (3 levels) in <top (required)>'

Finished in 0.08328 seconds (files took 0.08653 seconds to load)
94 examples, 2 failures

@RohanM RohanM merged commit 2b805a3 into main Apr 21, 2024
1 check passed
@RohanM RohanM deleted the 16814-upmark-add-basic-nested-list-handling branch April 21, 2024 22:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants