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.

Sign up for GitHub

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

Jump to bottom

Commonmarker 1.0 support #1540

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Commonmarker 1.0 support #1540

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2c0bac2
Surface missing/incompatible markups rather than skip
ParadoxV5 Mar 22, 2024
3472342
Support `Commonmarker` (lowercase `m`) from v1.x
ParadoxV5 Mar 23, 2024
0e274a0
Disable `Commonmarker` syntax highlighting for compatibility
ParadoxV5 Mar 23, 2024
9e869c9
Remove tests for Markdown header IDs or the lack of
ParadoxV5 Mar 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions lib/yard/templates/helpers/html_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,9 @@ def html_markup_markdown(text)
:tables,
:with_toc_data,
:no_intraemphasis).to_html
when 'Commonmarker'
# GFM configs are on by default; use YARD for syntax highlighting
Commonmarker.to_html(text, plugins: {syntax_highlighter: nil})
Copy link
Author

Choose a reason for hiding this comment

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

Suggested change
Commonmarker.to_html(text, plugins: {syntax_highlighter: nil})
Commonmarker.to_html(text)

However, preference for the provider’s highlighting is outside of this PR’s scope.

when 'CommonMarker'
CommonMarker.render_html(text, %i[DEFAULT GITHUB_PRE_LANG], %i[autolink table])
else
Expand Down
1 change: 1 addition & 0 deletions lib/yard/templates/helpers/markup_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ def clear_markup_cache
{:lib => :maruku, :const => 'Maruku'},
{:lib => :'rpeg-markdown', :const => 'PEGMarkdown'},
{:lib => :rdoc, :const => 'YARD::Templates::Helpers::Markup::RDocMarkdown'},
{:lib => :commonmarker, :const => 'Commonmarker'},
Copy link
Owner

Choose a reason for hiding this comment

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

This is problematic, as the lib value is a unique key passed via -m. This will effectively mean that the CommonMarker (pre 1.0?) class could never be activated. We should definitely maintain support for the old format, which would require this commonmarker key be changed-- it's not idela, but maybe :commonmarker1?

Copy link
Author

Choose a reason for hiding this comment

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

CommonMarker seems still accessible.

yard/lib/yard/templates/helpers/markup_helper.rb

Lines 91 to 110 in ad2c1c4

providers = MARKUP_PROVIDERS[type.to_sym]
return true if providers && providers.empty?
if providers && options.markup_provider
providers = providers.select {|p| p[:lib] == options.markup_provider }
end
if providers.nil? || providers.empty?
log.error "Invalid markup type '#{type}' or markup provider " \
"(#{options.markup_provider}) is not registered."
return false
end
# Search for provider, return the library class name as const if found
providers.each do |provider|
begin require provider[:lib].to_s; rescue LoadError; next end if provider[:lib]
begin klass = eval("::" + provider[:const]); rescue NameError; next end # rubocop:disable Lint/Eval
MarkupHelper.markup_cache[type][:provider] = provider[:lib] # Cache the provider
MarkupHelper.markup_cache[type][:class] = klass
return true
end

Even if it’s not, v1.0 CommonMaker should have priority over pre-1.0 Commonmarker.
Separating a commonmarker1 key keeps commonmarker out of date.
YARD does not have a system that checks provider versions and has long1 exposed users to Commonmarker 1.0’s backward incompatibility. We should improve this compatibility rather than make users work an extra step.

Footnotes

  1. Commonmarker 1.0 released last Christmas Eve! ↩

Copy link
Owner

Choose a reason for hiding this comment

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

Even if it’s not, v1.0 CommonMaker should have priority over pre-1.0 Commonmarker.

Unfortunately this would be a breaking change and would need a major version bump-- we're very unlikely to major version bump YARD just for a single (optional) markup library.

Copy link
Author

Choose a reason for hiding this comment

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

YARD is currently still in 0.x versions, so I’m not sure what you meant by ‘major’.

That said, together with #1540 (comment), I see your concern.
YARD currently has designs tailored for CommonMarker v0.x but it’s not documented anywhere.
We can only treat Commonmarker v1.x as a separate provider.

Copy link
Owner

Choose a reason for hiding this comment

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

YARD is currently still in 0.x versions, so I’m not sure what you meant by ‘major’.

YARD's 0.x releases are a major version. Believe it or not, there was a time before "SemVer", and YARD predates the existence of the now-popular semantic versioning specification, along with its arbitrary distinction that only 1.0+ are considered API stable. YARD's 0.x releases are considered a stable API. "0" is the current major version for YARD, we simply use 0-based indexing.

Copy link
Author

Choose a reason for hiding this comment

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

YARD does not have a system that checks provider versions

We might wanna add one. But for now, …

YARD currently has designs tailored for CommonMarker v0.x but it’s not documented anywhere.

… I’ll add a comment in the code since it’s now linked in the README.

Copy link
Author

Choose a reason for hiding this comment

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

At first glance it seems like YARD is no longer able to support the pre 1.0 commonmarker lib due to a change in the markup_helper lookup table.

Actually, we might not be able to support both Commonmarker and CommomMarker at all.
Commonmarker 1.x shadows CommonMarker 0.x. We’d have to write individual Bundler.requires in our specs.

{:lib => :commonmarker, :const => 'CommonMarker'}
],
:textile => [
Expand Down
2 changes: 1 addition & 1 deletion spec/templates/helpers/html_helper_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ def options

it "creates tables (markdown specific)" do
log.enter_level(Logger::FATAL) do
supports_table = %w(RedcarpetCompat Kramdown::Document CommonMarker)
supports_table = %w(RedcarpetCompat Kramdown::Document Commonmarker CommonMarker)
unless supports_table.include?(markup_class(:markdown).to_s)
pending "This test depends on a markdown engine that supports tables"
end
Expand Down
2 changes: 1 addition & 1 deletion spec/templates/markup_processor_integrations/integration_spec_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@

before(:each) do
if html_renderer.markup_class(markup).nil?
skip "Missing markup renderer #{markup}"
raise "Missing markup renderer #{markup}"
Copy link
Owner

Choose a reason for hiding this comment

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

This should stay as skip for systems that do not have the dependencies installed (testing outside of bundle exec).

Copy link
Author

@ParadoxV5 ParadoxV5 Aug 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I am unable to run rake/rspec locally without a prior bundle install.

Could not find redcarpet-3.6.0 in locally installed gems
Run `bundle install` to install missing gems.

While loading spec_helper an `exit` / `raise SystemExit` occurred, RSpec will now quit.
Failure/Error: require 'bundler/setup'

SystemExit:
  exit
# ./spec/spec_helper.rb:10:in `<top (required)>'
# ------------------
# --- Caused by: ---
# Bundler::GemNotFound:
#   Could not find redcarpet-3.6.0 in locally installed gems
#   ./spec/spec_helper.rb:10:in `<top (required)>'

yard/spec/spec_helper.rb

Lines 9 to 13 in ad2c1c4

begin
require 'bundler/setup'
rescue LoadError
nil # noop
end

end
end

Expand Down
11 changes: 1 addition & 10 deletions spec/templates/markup_processor_integrations/markdown_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,26 +72,17 @@

include_examples 'shared examples for markdown processors'


it 'generates anchor tags for level 2 header' do
expect(rendered_document).to include('<h2 id="example-code-listings">Example code listings</h2>')
end

it 'does not create line break via backslash' do
expect(rendered_document).to include("commonmark line break with\\\na backslash")
end
end

describe 'CommonMarker', if: RUBY_VERSION >= '2.3' do
describe 'Commonmarker', if: RUBY_VERSION >= '2.3' do
let(:markup) { :markdown }
let(:markup_provider) { :commonmarker }

include_examples 'shared examples for markdown processors'

it 'generates level 2 header without id' do
Copy link
Owner

Choose a reason for hiding this comment

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

This should remain but should be scoped to CommonMarker

Copy link
Author

Choose a reason for hiding this comment

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

Why? Does it have an impact?

  • Testing providers’ implementation is outside of the scope of YARD.

  • Remove tests for Markdown header IDs or the lack of, considering they may improve with newer provider versions and that YARD shouldn’t constraint their growth

Copy link
Owner

Choose a reason for hiding this comment

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

Testing providers’ implementation is outside of the scope of YARD.

Not when we're explicitly testing system integration. The fact that this behavior changes is exactly why we have tests. An id-less h2 heading actually has implications for downstream YARD code, since we have special code paths for heading id attributes.

Actually, expanding this comment outward, it seems like this change removes all integration tests for CommonMarker, which means we're now only testing Commonmarker 1.0. This is also insufficient. We should keep the old tests for CommonMarker and add the new ones for this 1.0 version, not replace.

tl;dr we need to test both CommonMarker / Commonmarker implementations. Not just one. You can remove the h2 test for Commonmarker 1.0 if you want, but both libraries should be integration tested.

Copy link
Owner

Choose a reason for hiding this comment

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

Remove tests for Markdown header IDs or the lack of, considering they may improve with newer provider versions and that YARD shouldn’t constraint their growth

YARD relies on this IDs though, that's the issue, and that's why we test.

expect(rendered_document).to include('<h2>Example code listings</h2>')
end

it 'creates line break via backslash' do
expect(rendered_document).to include("commonmark line break with<br />\na backslash")
end
Expand Down