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

[Rouge 2] use Rouge::Formatters::HTMLLegacy and fix tests #6150

Merged
merged 3 commits into from Jun 20, 2017

Conversation

Crunch09
Copy link
Member

Hey,
i changed Formatters::HTML for Formatters::HTMLLegacy based on @jneen 's suggestion (#5919 (comment)) and because Formatters::HTML (the rouge 2 version) doesn't print line numbers (unless i missed something).

Note: This targets #5919 instead of master. Will close #5230 in favor of this.

@DirtyF DirtyF requested a review from parkr June 18, 2017 05:40
@DirtyF DirtyF added this to the 3.6 milestone Jun 18, 2017
jekyll.gemspec Outdated
@@ -38,6 +38,6 @@ Gem::Specification.new do |s|
s.add_runtime_dependency("liquid", "~> 4.0")
s.add_runtime_dependency("mercenary", "~> 0.3.3")
s.add_runtime_dependency("pathutil", "~> 0.9")
s.add_runtime_dependency("rouge", "~> #{ENV["ROUGE_VERSION"] || "1.7"}")
s.add_runtime_dependency("rouge", "~> #{ENV["ROUGE_VERSION"] || "2.1.0"}")
Copy link
Member

Choose a reason for hiding this comment

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

Does this work with 2.0 & 2.2, or is only 2.1.x compatible? Usually we'd just use 2 digits here, e.g. 2.0.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 It should also work with any 2.x version, i'll update!

if old_api?
::Rouge::Formatters::HTML.new(*args)
else
::Rouge::Formatters::HTMLLegacy.new(*args)
Copy link
Member

Choose a reason for hiding this comment

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

What does the new API look like? is this just a wrapper around the new functionality with the old API?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, exactly. It produces (almost) the same output as rouge 1.11 while receiving the same arguments.

Copy link
Member Author

Choose a reason for hiding this comment

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

Here are all default formatters listed. IMHO line numbers (which we rely on in a couple of tests) would also be present in Rouge::Formatters::HTMLTable and Rouge::Formatters::HTMLLinewise but HTMLLegacy should be the easiest update.

Copy link
Member

Choose a reason for hiding this comment

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

Does HTMLLegacy not have those features?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, sorry, this was unclear: HTMLLegacy also has line numbers and all features we currently use.
The 2.x version of the standard HTML formatter doesn't have line numbers.

@@ -38,6 +38,6 @@ Gem::Specification.new do |s|
s.add_runtime_dependency("liquid", "~> 4.0")
s.add_runtime_dependency("mercenary", "~> 0.3.3")
s.add_runtime_dependency("pathutil", "~> 0.9")
s.add_runtime_dependency("rouge", "~> #{ENV["ROUGE_VERSION"] || "1.7"}")
s.add_runtime_dependency("rouge", "~> #{ENV["ROUGE_VERSION"] || "2.1"}")
Copy link
Contributor

@envygeeks envygeeks Jun 19, 2017

Choose a reason for hiding this comment

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

This is a bit wonky IMO. You could probably do something like:

s.add_runtime_dependency "rouge", ">= 1.7", "< 2.2"

And have a much easier time.

@parkr
Copy link
Member

parkr commented Jun 20, 2017

I'll fix up the dependency in the parent PR. Thank you!!!

@parkr parkr merged commit 2b20d10 into jekyll:rouge-1-and-2 Jun 20, 2017
@Crunch09 Crunch09 deleted the rouge-2-update branch June 20, 2017 22:17
@jekyll jekyll locked and limited conversation to collaborators Jul 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants