Rubocop: tags #4938

Merged
merged 7 commits into from May 26, 2016

Conversation

Projects
None yet
4 participants
@ayastreb
Contributor

ayastreb commented May 24, 2016

#4885

@ayastreb

This comment has been minimized.

Show comment
Hide comment
@ayastreb

ayastreb May 25, 2016

Contributor

Finished fixing all tags.
@pathawks , @envygeeks what do you think?

Contributor

ayastreb commented May 25, 2016

Finished fixing all tags.
@pathawks , @envygeeks what do you think?

lib/jekyll/tags/highlight.rb
- " returned an unacceptable value."
- Jekyll.logger.error "This is usually a timeout problem solved by running `jekyll build` again."
- raise ArgumentError.new("Pygments.rb returned an unacceptable value when attempting to highlight some code.")
+ Jekyll.logger.error <<-eos

This comment has been minimized.

@pathawks

pathawks May 25, 2016

Member

Oh wow, this is much improved! 😳👍

@pathawks

pathawks May 25, 2016

Member

Oh wow, this is much improved! 😳👍

lib/jekyll/tags/highlight.rb
- formatter = Rouge::Formatters::HTML.new(:line_numbers => @highlight_options[:linenos], :wrap => false)
+ Jekyll::External.require_with_graceful_fail("rouge")
+ formatter = Rouge::Formatters::HTML.new(
+ :line_numbers => @highlight_options[:linenos], :wrap => false

This comment has been minimized.

@pathawks

pathawks May 25, 2016

Member

Is it better to have each on it's own line?

formatter = Rouge::Formatters::HTML.new(
  :line_numbers => @highlight_options[:linenos],
  :wrap => false
)
@pathawks

pathawks May 25, 2016

Member

Is it better to have each on it's own line?

formatter = Rouge::Formatters::HTML.new(
  :line_numbers => @highlight_options[:linenos],
  :wrap => false
)

This comment has been minimized.

@ayastreb

ayastreb May 25, 2016

Contributor

Yes, I think each argument on it's own line would be better 👍

@ayastreb

ayastreb May 25, 2016

Contributor

Yes, I think each argument on it's own line would be better 👍

lib/jekyll/tags/link.rb
+ raise ArgumentError, <<eos
+Could not find document '#{@relative_path}' in tag '#{TAG_NAME}'.
+
+

This comment has been minimized.

@pathawks

pathawks May 25, 2016

Member

Do we want two blank lines here, or just one?

It looks like the first \n terminated the first line, and the second \n inserted one blank line, but I might be reading it wrong.

@pathawks

pathawks May 25, 2016

Member

Do we want two blank lines here, or just one?

It looks like the first \n terminated the first line, and the second \n inserted one blank line, but I might be reading it wrong.

This comment has been minimized.

@ayastreb

ayastreb May 25, 2016

Contributor

Yes, right! I'll remove the extra line 😄

@ayastreb

ayastreb May 25, 2016

Contributor

Yes, right! I'll remove the extra line 😄

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks May 25, 2016

Member

Thank you so much for all your hard work on this! Some of those changes make a huge improvement in readability! 🍻

Member

pathawks commented May 25, 2016

Thank you so much for all your hard work on this! Some of those changes make a huge improvement in readability! 🍻

@ayastreb

This comment has been minimized.

Show comment
Hide comment
@ayastreb

ayastreb May 25, 2016

Contributor

Hm, Travis build fails with rubocop error in file drops/drop.rb but I didn't change anything there 😕
Was it changed in master branch?

Contributor

ayastreb commented May 25, 2016

Hm, Travis build fails with rubocop error in file drops/drop.rb but I didn't change anything there 😕
Was it changed in master branch?

else
- raise SyntaxError.new <<-eos
+ raise SyntaxError, <<-eos

This comment has been minimized.

@envygeeks

envygeeks May 25, 2016

Contributor

I think we provide a heredoc stripper in the Utils class so that you can indent. I digress though on it's usage, I like to use it but if you prefer not to I'm okay with that too!

@envygeeks

envygeeks May 25, 2016

Contributor

I think we provide a heredoc stripper in the Utils class so that you can indent. I digress though on it's usage, I like to use it but if you prefer not to I'm okay with that too!

lib/jekyll/tags/highlight.rb
+
+ def parse_options(input)
+ options = {}
+ if defined?(input) && input != ""

This comment has been minimized.

@envygeeks

envygeeks May 25, 2016

Contributor

input will always be defined because it's in the signature.

@envygeeks

envygeeks May 25, 2016

Contributor

input will always be defined because it's in the signature.

This comment has been minimized.

@ayastreb

ayastreb May 25, 2016

Contributor

Do we need that check at all? Maybe it would be enough to do it like this:
unless input == ""

@ayastreb

ayastreb May 25, 2016

Contributor

Do we need that check at all? Maybe it would be enough to do it like this:
unless input == ""

This comment has been minimized.

@pathawks

pathawks May 25, 2016

Member
unless input.empty?

?

@pathawks

pathawks May 25, 2016

Member
unless input.empty?

?

@@ -110,13 +124,14 @@ def render_codehighlighter(code)
def add_code_tag(code)
code_attributes = [
- "class=\"language-#{@lang.to_s.tr('+', '-')}\"",
+ "class=\"language-#{@lang.to_s.tr("+", "-")}\"",

This comment has been minimized.

@envygeeks

envygeeks May 25, 2016

Contributor

It's preferred to use %Q{} here, did it not warn about this? I don't know if Rubocop can but it it didn't we should probably check and see if it can, I'll admit though that this might be a limitation in static analysis (it should be though IMO.)

@envygeeks

envygeeks May 25, 2016

Contributor

It's preferred to use %Q{} here, did it not warn about this? I don't know if Rubocop can but it it didn't we should probably check and see if it can, I'll admit though that this might be a limitation in static analysis (it should be though IMO.)

This comment has been minimized.

@ayastreb

ayastreb May 25, 2016

Contributor

No, it didn't warn, Rubocop only noticed single quotes here.
Sorry, I'm not too deep in Ruby yet, trying to learn it here 😄
Do you suggest to create this code tag with single interpolation string?
Like
%Q{<figure class="highlight"><pre><code class="language-#{@lang.to_s.tr("+", "-")}" data-lang="#{@lang}">#{code.chomp}</code></pre></figure>}
? It would be too long then. And if I change it to multiline string with heredoc then a lot of tests are failing because they expect a single line string.
Should we change it to multiline here and update the tests?

@ayastreb

ayastreb May 25, 2016

Contributor

No, it didn't warn, Rubocop only noticed single quotes here.
Sorry, I'm not too deep in Ruby yet, trying to learn it here 😄
Do you suggest to create this code tag with single interpolation string?
Like
%Q{<figure class="highlight"><pre><code class="language-#{@lang.to_s.tr("+", "-")}" data-lang="#{@lang}">#{code.chomp}</code></pre></figure>}
? It would be too long then. And if I change it to multiline string with heredoc then a lot of tests are failing because they expect a single line string.
Should we change it to multiline here and update the tests?

"data-lang=\"#{@lang}\""
].join(" ")
- "<figure class=\"highlight\"><pre><code #{code_attributes}>#{code.chomp}</code></pre></figure>"
+ "<figure class=\"highlight\"><pre><code #{code_attributes}>"\

This comment has been minimized.

@envygeeks

envygeeks May 25, 2016

Contributor

""

@envygeeks

envygeeks May 25, 2016

Contributor

""

lib/jekyll/tags/include.rb
else
- @file, @params = markup.strip.split(' ', 2)
+ @file, @params = markup.strip.split(" ", 2)

This comment has been minimized.

@envygeeks

envygeeks May 25, 2016

Contributor

This might be a good time to fix that bad split. Users are prone to alignment because it's prettier, if you don't mind can you make this markup.strip.split(/\s+/, 2) that way users can infinity space and align any way they see fit. I don't know if it's worth it though considering it's not been a problem before?

@envygeeks

envygeeks May 25, 2016

Contributor

This might be a good time to fix that bad split. Users are prone to alignment because it's prettier, if you don't mind can you make this markup.strip.split(/\s+/, 2) that way users can infinity space and align any way they see fit. I don't know if it's worth it though considering it's not been a problem before?

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks May 25, 2016

Contributor

Thanks! Just a few comments!

Contributor

envygeeks commented May 25, 2016

Thanks! Just a few comments!

@parkr parkr merged commit 001cbf2 into jekyll:master May 26, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

parkr added a commit that referenced this pull request May 26, 2016

@ayastreb ayastreb deleted the ayastreb:tags branch May 27, 2016

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