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

Drop support for pygments as syntax-highlighter #6983

Closed
wants to merge 8 commits into from
Closed

Drop support for pygments as syntax-highlighter #6983

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f887b28
Drop support for pygments as syntax-highlighter
ashmaroli May 3, 2018
dc20331
remove pygments.rb listed in the Gemfile
ashmaroli May 4, 2018
80fe6aa
Merge upstream 'master' into drop-pygments
ashmaroli May 6, 2018
930cd94
remove unused method in HighlightBlock
ashmaroli May 7, 2018
ca5ee85
default highlighter takes precedence
ashmaroli May 8, 2018
b99056a
fallback to using Rouge with some warnings
ashmaroli May 8, 2018
f1a6e12
Merge upstream 'master' into drop-pygments
ashmaroli May 11, 2018
bf41a6e
Merge upstream 'master' into drop-pygments
ashmaroli Jul 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 0 additions & 1 deletion Gemfile
View file
Open in desktop
Expand Up @@ -75,7 +75,6 @@ group :jekyll_optional_dependencies do
platform :ruby, :mswin, :mingw, :x64_mingw do
gem "classifier-reborn", "~> 2.2.0"
gem "liquid-c", "~> 3.0"
gem "pygments.rb", "~> 1.0"
gem "rdiscount", "~> 2.0"
gem "yajl-ruby", "~> 1.3"
end
Expand Down
8 changes: 0 additions & 8 deletions features/site_configuration.feature
View file
Open in desktop
Expand Up @@ -81,14 +81,6 @@ Feature: Site configuration
And the _site directory should exist
And I should see "<a href=\"https://www.google.com\">Google</a>" in "_site/index.html"

Scenario: Highlight code with pygments
Given I have an "index.html" page that contains "{% highlight ruby %} puts 'Hello world!' {% endhighlight %}"
When I run jekyll build
Then I should get a zero exit status
And the _site directory should exist
And I should see "Hello world!" in "_site/index.html"
And I should see "class=\"highlight\"" in "_site/index.html"

Scenario: Highlight code with rouge
Given I have an "index.html" page that contains "{% highlight ruby %} puts 'Hello world!' {% endhighlight %}"
And I have a configuration file with "highlighter" set to "rouge"
Expand Down
48 changes: 7 additions & 41 deletions lib/jekyll/tags/highlight.rb
View file
Open in desktop
Expand Up @@ -33,14 +33,12 @@ def render(context)
suffix = context["highlighter_suffix"] || ""
code = super.to_s.gsub(%r!\A(\n|\r)+|(\n|\r)+\z!, "")

is_safe = !!context.registers[:site].safe

output =
case context.registers[:site].highlighter
when "pygments"
render_pygments(code, is_safe)
when "rouge"
render_rouge(code)
when "pygments"
render_pygments(code, context)
else
render_codehighlighter(code)
end
Expand All @@ -49,20 +47,6 @@ def render(context)
prefix + rendered_output + suffix
end

def sanitized_opts(opts, is_safe)
if is_safe
Hash[[
[:startinline, opts.fetch(:startinline, nil)],
[:hl_lines, opts.fetch(:hl_lines, nil)],
[:linenos, opts.fetch(:linenos, nil)],
[:encoding, opts.fetch(:encoding, "utf-8")],
[:cssclass, opts.fetch(:cssclass, nil)],
].reject { |f| f.last.nil? }]
else
opts
end
end

private

OPTIONS_REGEX = %r!(?:\w="[^"]*"|\w=\w|\w)+!
Expand All @@ -86,29 +70,11 @@ def parse_options(input)
options
end

def render_pygments(code, is_safe)
Jekyll::External.require_with_graceful_fail("pygments") unless defined?(Pygments)

highlighted_code = Pygments.highlight(
code,
:lexer => @lang,
:options => sanitized_opts(@highlight_options, is_safe)
)

if highlighted_code.nil?
Jekyll.logger.error <<-MSG
There was an error highlighting your code:

#{code}

While attempting to convert the above code, Pygments.rb returned an unacceptable value.
This is usually a timeout problem solved by running `jekyll build` again.
MSG
raise ArgumentError, "Pygments.rb returned an unacceptable value "\
"when attempting to highlight some code."
end

highlighted_code.sub('<div class="highlight"><pre>', "").sub("</pre></div>", "")
def render_pygments(code, _context)
Jekyll.logger.warn "Warning:",
Copy link
Member

Choose a reason for hiding this comment

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

This should probably fail the build, right?

Otherwise, folks who use (ie) GitHub Pages will just stop having syntax highlighting in their code with no clue as to why, and that is if they notice.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah.. that's true..
I'd prefer users to keep abreast with changes in a major version and check for any warnings in their terminal output instead of getting a Page Build Error email from GitHub Pages..
but that's just me..

People just don't have the time to do a local run first.. 🙂
I'll update it to raise instead

Copy link
Member Author

Choose a reason for hiding this comment

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

@pathawks Or... should we silently fallback to highlighting with Rouge? 💡

Copy link
Member

Choose a reason for hiding this comment

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

@ashmaroli if there is no compatibility issue between the two highlighters, this could be the nicest solution for all users.

Copy link
Member Author

Choose a reason for hiding this comment

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

From the README at jneen/rouge:

Its HTML output is compatible with stylesheets designed for pygments.

If there's still a need for using pygments explicitly, the community can build a plugin which will be able to easily hook into the render method here.

Copy link
Member

Choose a reason for hiding this comment

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

Rouge::Formatters::HTMLPygments.new(formatter, css_class='codehilite') wraps the given formatter with div wrappers generally expected by stylesheets designed for Pygments

"Highlight Tag no longer supports rendering with Pygments."
Jekyll.logger.warn "", "Using the default highlighter, Rouge, instead."
render_rouge(code)
end

def render_rouge(code)
Expand Down
176 changes: 0 additions & 176 deletions test/test_tags.rb
View file
Open in desktop
Expand Up @@ -134,182 +134,6 @@ def highlight_block_with_opts(options_string)
end
end

context "in safe mode" do
setup do
@tag = highlight_block_with_opts("text ")
end

should "allow linenos" do
sanitized = @tag.sanitized_opts({ :linenos => true }, true)
assert_equal true, sanitized[:linenos]
end

should "allow hl_lines" do
sanitized = @tag.sanitized_opts({ :hl_lines => %w(1 2 3 4) }, true)
assert_equal %w(1 2 3 4), sanitized[:hl_lines]
end

should "allow cssclass" do
sanitized = @tag.sanitized_opts({ :cssclass => "ahoy" }, true)
assert_equal "ahoy", sanitized[:cssclass]
end

should "allow startinline" do
sanitized = @tag.sanitized_opts({ :startinline => true }, true)
assert_equal true, sanitized[:startinline]
end

should "strip unknown options" do
sanitized = @tag.sanitized_opts({ :light => true }, true)
assert_nil sanitized[:light]
end
end

context "with the pygments highlighter" do
setup do
if jruby?
then skip(
"JRuby does not support Pygments."
)
end
end

context "post content has highlight tag" do
setup do
fill_post("test", { "highlighter" => "pygments" })
end

should "not cause a markdown error" do
refute_match(%r!markdown\-html\-error!, @result)
end

should "render markdown with pygments" do
assert_match(
%(<pre><code class="language-text" data-lang="text">) +
%(<span></span>test</code></pre>),
@result
)
end

should "render markdown with pygments with line numbers" do
assert_match(
%(<pre><code class="language-text" data-lang="text">) +
%(<span></span><span class="lineno">1 </span>test</code></pre>),
@result
)
end
end

context "post content has highlight with file reference" do
setup do
fill_post("./jekyll.gemspec", { "highlighter" => "pygments" })
end

should "not embed the file" do
assert_match(
%(<pre><code class="language-text" data-lang="text"><span></span>) +
%(./jekyll.gemspec</code></pre>),
@result
)
end
end

context "post content has highlight tag with UTF character" do
setup do
fill_post("Æ", { "highlighter" => "pygments" })
end

should "render markdown with pygments line handling" do
assert_match(
%(<pre><code class="language-text" data-lang="text">) +
%(<span></span>Æ</code></pre>),
@result
)
end
end

context "post content has highlight tag with preceding spaces & lines" do
setup do
code = <<-EOS


[,1] [,2]
[1,] FALSE TRUE
[2,] FALSE TRUE
EOS
fill_post(code, { "highlighter" => "pygments" })
end

should "only strip the preceding newlines" do
assert_match(
%(<pre><code class=\"language-text\" data-lang=\"text\">) +
%(<span></span> [,1] [,2]),
@result
)
end
end

context "post content has highlight tag " \
"with preceding spaces & lines in several places" do
setup do
code = <<-EOS


[,1] [,2]


[1,] FALSE TRUE
[2,] FALSE TRUE


EOS
fill_post(code, { "highlighter" => "pygments" })
end

should "only strip the newlines which precede and succeed the entire block" do
assert_match(
%(<pre><code class=\"language-text\" data-lang=\"text\"><span></span>) +
%( [,1] [,2]\n\n\n[1,] FALSE TRUE\n[2,] FALSE TRUE</code></pre>),
@result
)
end
end

context "post content has highlight tag with " \
"preceding spaces & Windows-style newlines" do
setup do
fill_post "\r\n\r\n\r\n [,1] [,2]", { "highlighter" => "pygments" }
end

should "only strip the preceding newlines" do
assert_match(
%(<pre><code class="language-text" data-lang="text"><span></span>) +
%( [,1] [,2]),
@result
)
end
end

context "post content has highlight tag with only preceding spaces" do
setup do
code = <<-EOS
[,1] [,2]
[1,] FALSE TRUE
[2,] FALSE TRUE
EOS
fill_post(code, { "highlighter" => "pygments" })
end

should "only strip the preceding newlines" do
assert_match(
%(<pre><code class=\"language-text\" data-lang=\"text\"><span></span>) +
%( [,1] [,2]),
@result
)
end
end
end

context "with the rouge highlighter" do
context "post content has highlight tag" do
setup do
Expand Down