Update Rubocop to 0.51.0 by jekyllbot · Pull Request #6444 · jekyll/jekyll

jekyllbot · 2017-10-19T03:10:43Z

PR automatically created for @pathawks.

Update Rubocop to 0.51.0

Fixes #6441

Fixes #6441

pathawks · 2017-10-19T03:14:33Z

Hey @ashmaroli 👋
I would request a review from you, but you don't seem to be listed. 🤷‍♂️

pathawks · 2017-10-19T03:15:36Z

.rubocop.yml

 Style/DoubleNegation:
  Enabled: false
-Style/Encoding:
-  EnforcedStyle: when_needed


Error: obsolete parameter EnforcedStyle (for Style/Encoding) found in .rubocop.yml Style/Encoding no longer supports styles. The "never" behavior is always assumed.

I'd like us to have the special encoding header on all our files if possible.

@parkr The Ruby Interpreter from v2.0 onwards encodes all files as UTF-8..

from the Ruby docs:

The related Rubocop commit: rubocop/rubocop@5faf140

pathawks · 2017-10-19T03:16:16Z

.rubocop.yml

 ---
 AllCops:
-  TargetRubyVersion: 2.0
+  TargetRubyVersion: 2.1


Error: Unsupported Ruby version 2.0 found in `TargetRubyVersion` parameter (in .rubocop.yml). 2.0-compatible analysis was dropped after version 0.50. Supported versions: 2.1, 2.2, 2.3, 2.4

Jekyll itself dropped support for 2.0. So this is in sync..

pathawks · 2017-10-19T03:17:00Z

lib/jekyll/commands/build.rb

          end
        end
-      end # end of class << self
+      end


lib/jekyll/commands/build.rb:99:11: C: Style/CommentedKeyword: Do not place comments on the same line as the end keyword. end # end of class << self ^^^^^^^^^^^^^^^^^^^^^^

pathawks · 2017-10-19T03:17:20Z

lib/jekyll/configuration.rb

        Jekyll.logger.warn "WARNING:", "Error reading configuration. " \
                     "Using defaults (and options)."
-        $stderr.puts err
+        warn err


lib/jekyll/configuration.rb:210:9: C: Style/StderrPuts: Use warn instead of $stderr.puts. $stderr.puts err ^^^^^^^^^^^^

can these be changed to use Jekyll's logger instead?

I wonder if we could just append it to the string we are already logging?

Revisiting this, I'm not sure if we should be changing an outcome of a public method.. (..in the rare case someone's capturing $stderr for some purpose..)

So I think its best that we stick to warn in this PR or disable that check entirely if that's better..

Would it be changing behavior? I'm sure our logger outputs to stderr, right?

pathawks · 2017-10-19T03:17:38Z

lib/jekyll/converters/markdown/redcarpet_parser.rb


  module WithRouge
-    def block_code(code, lang)
+    def block_code(_code, lang)


lib/jekyll/converters/markdown/redcarpet_parser.rb:50:20: W: Lint/UnusedMethodArgument: Unused method argument - code. If it's necessary, use _ or _code as an argument name to indicate that it won't be used. def block_code(code, lang) ^^^^

Hmm... this looks like a Rubocop bug, as code is clearly used in line 53.

@pathawks it doesn't look like a Rubocop bug to me since if you look at line 51, code is being immediately re-assigned with code = "<pre>#{super}</pre>" so in effect overriding whatever has been passed to :block_code as the first argument..

You are right! It is doing exactly what it’s supposed to do. We don’t need this.

pathawks · 2017-10-19T03:17:54Z

test/test_filters.rb

@@ -1,4 +1,3 @@
-# coding: utf-8


test/test_filters.rb:1:1: C: Style/Encoding: Unnecessary utf-8 encoding comment. # coding: utf-8 ^^^^^^^^^^^^^^^

ashmaroli · 2017-10-19T03:50:30Z

@pathawks Thanks for the mention 😃
AFAIC, I'm in agreement with all the changes proposed here, except for replacing $stderr.puts with warn.. Wonder if there's another alternative..

pathawks · 2017-10-19T18:22:23Z

Ok, I am going to merge this for now because every project that inherits jekyll/jekyll is currently borked.

Maybe in another PR we can revisit @ashmaroli's idea of cleaning up warn.

pathawks · 2017-10-19T18:22:35Z

@jekyllbot: merge +dev

ashmaroli · 2017-10-20T04:05:20Z

because every project that inherits jekyll/jekyll is currently borked.

We had locked Rubocop to use ~> 0.50.0 (before merge). Is it not the dependent repo's fault for not locking to the patch release themselves?

pathawks · 2017-10-20T04:06:53Z

Is it not the dependent repo's fault for not locking to the patch release themselves?

I think I was trying to use the latest version of Rubocop with a plugin. You are correct, of course.

DirtyF · 2017-10-20T07:34:22Z

I just did a test with the jekyll-watch plugin which is loose on the minor version:
spec.add_development_dependency "rubocop", "~> 0.5"

~/code/jekyll/plugins/jekyll-watch master
❯ script/fmt
/Users/frank/.rbenv/versions/2.4.2/lib/ruby/gems/2.4.0/gems/jekyll-3.6.0/.rubocop.yml: Style/FileName has the wrong namespace - should be Naming
Error: Unsupported Ruby version 2.0 found in `TargetRubyVersion` parameter (in /Users/frank/.rbenv/versions/2.4.2/lib/ruby/gems/2.4.0/gems/jekyll-3.6.0/.rubocop.yml). 2.0-compatible analysis was dropped after version 0.50.
Supported versions: 2.1, 2.2, 2.3, 2.4

Does this justify a 3.6.1 release?

pathawks · 2017-10-20T13:21:14Z

@DirtyF Yes but—

We have merged minor changes in master, so easiest thing would be to cut a 3.7.0 release.
Other option would be to create a stable-3.6 branch and backport this change.

Up to you.

pathawks · 2017-10-20T14:13:47Z

I had no idea this was a thing!! https://github.com/jekyll/jekyll/blob/3.6-stable/History.markdown

@jekyllbot I love you.

pathawks · 2017-10-20T14:27:18Z

Tada 🎉

pathawks · 2017-10-20T14:30:27Z

Oh, no... I got way ahead of myself. That didn't do what I thought it was going to do.

DirtyF · 2017-10-20T15:30:20Z

@pathawks I'm not sure what's goin' on here.

~/code/jekyll/plugins/jekyll-watch master
$ bundle update
$ …
$ Installing jekyll 3.6.1 (was 3.6.0)
$ Bundle updated!
~/code/jekyll/plugins/jekyll-watch master
$ script/fmt
Error: obsolete parameter EnforcedStyle (for Style/Encoding) found in /Users/frank/.rbenv/versions/2.4.2/lib/ruby/gems/2.4.0/gems/jekyll-3.6.1/.rubocop.yml
Style/Encoding no longer supports styles. The "never" behavior is always assumed.

@parkr said plugins will need some love, I'm up for creating first-timer- branches given I know exactly how to fix this.

parkr · 2017-10-20T15:40:33Z

Made some notes here about how I approach the release & development process: https://github.com/jekyll/maintainers/issues/2#issuecomment-338017913

pathawks · 2017-10-20T15:45:03Z

3.6.1 doesn’t include that Rubocop change. 3.6.2 will.

Sorry; I’m still learning how all this works.

DirtyF · 2017-10-20T15:46:47Z

@pathawks same here, just trying to catch on what's goin' on. You're doing great.

pathawks · 2017-10-20T16:05:28Z

@DirtyF I saw that there was already a Rubocop PR in the 3.6-stable branch and assumed that @jekyllbot had been automatically backporting merged PRs.

This was not the case, so 3.6.1 lacks what we need.

Update Rubocop to 0.51.0

fc0fe7e

Fixes #6441

pathawks changed the title ~~Update Rubocop to 0.51.0 Fixes #6441~~ Update Rubocop to 0.51.0 Oct 19, 2017

pathawks requested a review from DirtyF October 19, 2017 03:11

pathawks reviewed Oct 19, 2017

View reviewed changes

DirtyF approved these changes Oct 19, 2017

View reviewed changes

jekyllbot merged commit e7f1ce2 into master Oct 19, 2017

jekyllbot added fix internal labels Oct 19, 2017

jekyllbot deleted the pull/rubocop branch October 19, 2017 18:22

jekyllbot added a commit that referenced this pull request Oct 19, 2017

Update history to reflect merge of #6444 [ci skip]

28af8bd

Crunch09 mentioned this pull request Oct 19, 2017

Update dependency constraint to allow for rubocop v0.51.0 #6441

Closed

benbalter mentioned this pull request Oct 20, 2017

Spike out jekyll-remote-theme benbalter/jekyll-remote-theme#1

Merged

pathawks pushed a commit that referenced this pull request Oct 20, 2017

Backport pull/rubocop from #6444 to 3.6-stable

e873d5e

pathawks pushed a commit that referenced this pull request Oct 20, 2017

Backport pull/rubocop from #6444 to 3.6-stable

4e06289

pathawks added a commit that referenced this pull request Oct 20, 2017

Backport pull/rubocop from #6444 to 3.6-stable (#6450)

5389762

pathawks mentioned this pull request Oct 20, 2017

Release 3.6.2 #6454

Closed

jekyll locked and limited conversation to collaborators Jul 12, 2019

jekyllbot added the frozen-due-to-age label Jul 12, 2019

Uh oh!

Conversation

jekyllbot commented Oct 19, 2017

Uh oh!

pathawks commented Oct 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ashmaroli Oct 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

from the Ruby docs:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ashmaroli commented Oct 19, 2017

Uh oh!

pathawks commented Oct 19, 2017

Uh oh!

pathawks commented Oct 19, 2017

Uh oh!

ashmaroli commented Oct 20, 2017

Uh oh!

pathawks commented Oct 20, 2017

Uh oh!

DirtyF commented Oct 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pathawks commented Oct 20, 2017

Uh oh!

pathawks commented Oct 20, 2017

Uh oh!

pathawks commented Oct 20, 2017

Uh oh!

pathawks commented Oct 20, 2017

Uh oh!

DirtyF commented Oct 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

parkr commented Oct 20, 2017

Uh oh!

pathawks commented Oct 20, 2017

Uh oh!

DirtyF commented Oct 20, 2017

Uh oh!

pathawks commented Oct 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

ashmaroli Oct 20, 2017 •

edited

Loading

DirtyF commented Oct 20, 2017 •

edited

Loading

DirtyF commented Oct 20, 2017 •

edited

Loading