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

Only complain about `kramdown.coderay` if it is actually in the config #5380

Merged
merged 1 commit into from Sep 20, 2016

Conversation

Projects
None yet
4 participants
@ethomson
Contributor

ethomson commented Sep 19, 2016

After upgrading to jekyll 3, I wanted to continue using coderay as my highlighter. So I updated my configuration to:

kramdown:
  syntax_highlighter: coderay
  syntax_highlighter_opts:
    line_numbers: inline
    css: class

And I was surprised when jekyll complained:

Deprecation: You are using 'kramdown.coderay' in your configuration, please use 'syntax_highlighter_opts' instead.

It seems that jekyll is raising this notice whenever highlighter == "coderay", even when the kramdown.coderay configuration setting is nonexistent.

Switching this to check for a non-empty kramdown.coderay hash keeps this warning when the user has kramdown.coderay settings, but suppresses this warning when the configuration has switched to the new style kramdown.syntax_highlighter_opts.

Show outdated Hide outdated lib/jekyll/converters/markdown/kramdown_parser.rb
@@ -104,7 +104,7 @@ def strip_coderay_prefix(hash)
private
def modernize_coderay_config
if highlighter == "coderay"
if !@config["coderay"].empty?

This comment has been minimized.

@pathawks

pathawks Sep 19, 2016

Member

I can't speak to the wider implications of this change, but as far as style goes, could we use unless instead of if !?
I think Rubocop should be complaining about this.

Edit: Sure enough.

@pathawks

pathawks Sep 19, 2016

Member

I can't speak to the wider implications of this change, but as far as style goes, could we use unless instead of if !?
I think Rubocop should be complaining about this.

Edit: Sure enough.

This comment has been minimized.

@ethomson

ethomson Sep 19, 2016

Contributor

Sure; fixed.

@ethomson

ethomson Sep 19, 2016

Contributor

Sure; fixed.

Edward Thomson
Only complain about `coderay` if it is actually in the config
Don't complain about the deprecated `kramdown.coderay` key when
`highlighter == "coderay"`, since that could have been set with the
legitimate `syntax_highlighter: coderay` setting.  Instead, complain
only if the `kramdown.coderay` configuration setting is actually
present.
@parkr

parkr approved these changes Sep 20, 2016

Thanks, @ethomson! This LGTM. If I'm following the logic properly, then this message will only fire if the coderay configuration key is present with some non-empty value. Seem right?

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Sep 20, 2016

Member

Rubocop errors are fixed on master. We can merge without them.

Member

parkr commented Sep 20, 2016

Rubocop errors are fixed on master. We can merge without them.

@parkr parkr added bug fix labels Sep 20, 2016

@parkr parkr added this to the 3.3 milestone Sep 20, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Sep 20, 2016

Member

@jekyllbot: merge +buf

Member

parkr commented Sep 20, 2016

@jekyllbot: merge +buf

@jekyllbot jekyllbot merged commit cfe6177 into jekyll:master Sep 20, 2016

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details

jekyllbot added a commit that referenced this pull request Sep 20, 2016

parkr added a commit that referenced this pull request Sep 21, 2016

@ethomson

This comment has been minimized.

Show comment
Hide comment
@ethomson

ethomson Sep 21, 2016

Contributor

Thanks @parkr ! ❤️

Contributor

ethomson commented Sep 21, 2016

Thanks @parkr ! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment