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

Support both Commonmarker pre and post 1.0 #1601

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gjtorikian
Copy link

@gjtorikian gjtorikian commented Jan 27, 2025

Description

Hello! I maintain commonmarker, a Ruby gem for converting CommonMark markup into HTML.

Recently, @noraj opened an issue asking me about #1540. I don't know the entire history of the PR, but my understanding is that there are a few issues:

  • YARD should support both pre- and post 1.0 versions of Commonmarker
  • The CommonMarker const, which YARD uses, should not change, otherwise it's a breaking change
  • header tags need to be without IDs

In this PR, I am proposing that YARD just keep using whatever configuration values it accepts to configure commonmarker processing; at the time of processing, if the old CommonMarker const is available, it's implied that that version of commonmarker is being used, so YARD should call the method that version expects; otherwise, the newer version will be used.

The one case this doesn't cover is this one:

we need to test both CommonMarker / Commonmarker implementations.

Note that the current test suite passes, because unlike #1540 I passed the options necessary to make a 1:1 match to the old behavior. But: I don't know how to configure the tests or more specifically CI to load two versions of the gem—nor am I interested in learning how!

I think I've fulfilled my open source duties thus far. If I can help push this along in any way let me know. ✌️

Completed Tasks

  • I have read the Contributing Guide.
  • The pull request is complete (implemented / written).
  • Git commits have been cleaned up (squash WIP / revert commits).
  • I wrote tests and ran bundle exec rake locally (if code is attached to PR).

@@ -89,7 +89,9 @@ def html_markup_markdown(text)
:tables,
:with_toc_data,
:no_intraemphasis).to_html
when 'CommonMarker'
when 'Commonmarker' # GFM configs are on by default; use YARD for syntax highlighting
Copy link
Author

Choose a reason for hiding this comment

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

Maybe defined? needs to go here too? I didn't really follow the thread of how const is used.

It seems like its only usage is here, to check if the const is available: https://github.com/gjtorikian/yard/blob/cf44a71c6715e39c9868b2beddaa0be4cf67b71e/lib/yard/templates/helpers/markup_helper.rb#L106

But that just instantiates a class, which is turned into a string, which is used in this case statement.

It seems like you could drop the class instantiation and just pass a string here, because klass serves no purpose: https://github.com/gjtorikian/yard/blob/cf44a71c6715e39c9868b2beddaa0be4cf67b71e/lib/yard/templates/helpers/markup_helper.rb#L104-L110

But I don't know all the details of this project. 🤷

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.

1 participant