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

Replace Redcarpet by CommonMarker #419

Merged
merged 4 commits into from
Nov 16, 2023
Merged

Conversation

ania-hm
Copy link
Contributor

@ania-hm ania-hm commented Nov 15, 2023

Pull Request Summary

It is part of a larger task to add pages describing industries where we can help as programmers. CommonMarker has more features and is faster than Redcarpet. This helps for better rendering of HTML from Markdown.

Feedback

N/A

UI Changes

N/A

Copy link
Member

@torrocus torrocus left a comment

Choose a reason for hiding this comment

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

Please add tests to make sure the behavior is as expected.

@torrocus torrocus marked this pull request as draft November 15, 2023 19:04
@ania-hm ania-hm force-pushed the ab-replace-redcarpet-to-commonmarker branch from d5da141 to eaafce3 Compare November 16, 2023 10:44
spec/lib/markdown_spec.rb Outdated Show resolved Hide resolved
@ania-hm ania-hm force-pushed the ab-replace-redcarpet-to-commonmarker branch from eaafce3 to a35df6f Compare November 16, 2023 11:02
spec/lib/markdown_spec.rb Outdated Show resolved Hide resolved
@ania-hm ania-hm force-pushed the ab-replace-redcarpet-to-commonmarker branch from a35df6f to 7174310 Compare November 16, 2023 11:07
spec/lib/markdown_spec.rb Outdated Show resolved Hide resolved
Gemfile.lock Outdated Show resolved Hide resolved
spec/lib/markdown_spec.rb Outdated Show resolved Hide resolved
spec/lib/markdown_spec.rb Outdated Show resolved Hide resolved
spec/lib/markdown_spec.rb Outdated Show resolved Hide resolved
spec/lib/markdown_spec.rb Outdated Show resolved Hide resolved
lib/markdown.rb Outdated Show resolved Hide resolved
@ania-hm ania-hm force-pushed the ab-replace-redcarpet-to-commonmarker branch 2 times, most recently from d9588a5 to 46b32a7 Compare November 16, 2023 13:00
Copy link
Member

@torrocus torrocus left a comment

Choose a reason for hiding this comment

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

Thanks for the tests, it looks much better.

@ania-hm ania-hm force-pushed the ab-replace-redcarpet-to-commonmarker branch 2 times, most recently from 52ee84e to 1d61211 Compare November 16, 2023 13:37
@ania-hm ania-hm marked this pull request as ready for review November 16, 2023 13:39
Comment on lines 92 to 93
expected_result_in_html = '<p><a href="https://fractalsoft.org/">' \
"<img src=\"/path/to/my_image\" alt=\"My image\" /></a></p>\n"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
expected_result_in_html = '<p><a href="https://fractalsoft.org/">' \
"<img src=\"/path/to/my_image\" alt=\"My image\" /></a></p>\n"
expected_result_in_html = <<~HTML
<p><a href="https://fractalsoft.org/"><img src="/path/to/image" alt="My image" /></a></p>
HTML

@ania-hm ania-hm force-pushed the ab-replace-redcarpet-to-commonmarker branch from 1d61211 to 424a486 Compare November 16, 2023 13:58
We were missing tests to check
the conversion from Markdown to HTML.
While writing the missing tests,
I found out that some implementations for using Redcarpet didn't work.

We had added some Redcarpet extensions, [1]
but they did not load in the application when we used them.
To be more specific,
the Redcarpet extensions were not enabled in the application
where we were using them.

This was the reason for refactoring the code.

The tests that check for correct conversion of Markdown
to HTML with Redcarpet pass.

[1]: https://www.writesoftwarewell.com/how-to-render-markdown-views-in-rails/
> Ruby wrapper for the comrak (CommonMark parser) Rust crate [1]

[1]: https://github.com/gjtorikian/commonmarker
We replace Redcarpet with CommonMarker [1]
because the latter has more features and is faster. [2]
This helps to render HTML from Markdown better.

I had to refactor the tests due this change.

[1]: https://github.com/gjtorikian/commonmarker/blob/v1.0.0.pre11/README.md
[2]: https://github.github.com/gfm/
Since Redcarpet [1] is not used anywhere, I have removed it.

[1]: https://github.com/vmg/redcarpet
@ania-hm ania-hm force-pushed the ab-replace-redcarpet-to-commonmarker branch from 424a486 to aa89b81 Compare November 16, 2023 14:30
@ania-hm ania-hm merged commit 83cebae into main Nov 16, 2023
13 checks passed
@ania-hm ania-hm deleted the ab-replace-redcarpet-to-commonmarker branch November 16, 2023 14:38
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