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

Update Rubocop to 0.51.0 #6444

Merged
merged 1 commit into from Oct 19, 2017
Merged

Update Rubocop to 0.51.0 #6444

merged 1 commit into from Oct 19, 2017

Conversation

@jekyllbot
Copy link
Contributor

jekyllbot commented Oct 19, 2017

PR automatically created for @pathawks.

Update Rubocop to 0.51.0

Fixes #6441
Fixes #6441
@pathawks pathawks changed the title Update Rubocop to 0.51.0 Fixes #6441 Update Rubocop to 0.51.0 Oct 19, 2017
@pathawks pathawks requested a review from DirtyF Oct 19, 2017
@pathawks

This comment has been minimized.

Copy link
Member

pathawks commented Oct 19, 2017

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

@@ -117,8 +117,6 @@ Style/Documentation:
- !ruby/regexp /features\/.*.rb$/
Style/DoubleNegation:
Enabled: false
Style/Encoding:
EnforcedStyle: when_needed

This comment has been minimized.

Copy link
@pathawks

pathawks Oct 19, 2017

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

This comment has been minimized.

Copy link
@parkr

parkr Oct 20, 2017

Member

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

This comment has been minimized.

Copy link
@ashmaroli

ashmaroli Oct 20, 2017

Member

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

from the Ruby docs:

encoding

This comment has been minimized.

Copy link
@DirtyF

DirtyF Oct 20, 2017

Member

The related Rubocop commit: rubocop-hq/rubocop@5faf140

@@ -1,6 +1,6 @@
---
AllCops:
TargetRubyVersion: 2.0
TargetRubyVersion: 2.1

This comment has been minimized.

Copy link
@pathawks

pathawks Oct 19, 2017

Member
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

This comment has been minimized.

Copy link
@ashmaroli

ashmaroli Oct 19, 2017

Member

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

@@ -96,7 +96,7 @@ def watch(site, options)
)
end
end
end # end of class << self
end

This comment has been minimized.

Copy link
@pathawks

pathawks Oct 19, 2017

Member
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
          ^^^^^^^^^^^^^^^^^^^^^^
@@ -207,7 +207,7 @@ def read_config_files(files)
rescue ArgumentError => err
Jekyll.logger.warn "WARNING:", "Error reading configuration. " \
"Using defaults (and options)."
$stderr.puts err
warn err

This comment has been minimized.

Copy link
@pathawks

pathawks Oct 19, 2017

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

This comment has been minimized.

Copy link
@ashmaroli

ashmaroli Oct 19, 2017

Member

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

This comment has been minimized.

Copy link
@pathawks

pathawks Oct 19, 2017

Member

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

This comment has been minimized.

Copy link
@ashmaroli

ashmaroli Oct 19, 2017

Member

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..

This comment has been minimized.

Copy link
@pathawks

pathawks Oct 19, 2017

Member

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

@@ -47,7 +47,7 @@ def block_code(code, lang)
end

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

This comment has been minimized.

Copy link
@pathawks

pathawks Oct 19, 2017

Member
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)
                   ^^^^

This comment has been minimized.

Copy link
@pathawks

pathawks Oct 20, 2017

Member

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

This comment has been minimized.

Copy link
@ashmaroli

ashmaroli Oct 21, 2017

Member

@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..

This comment has been minimized.

Copy link
@pathawks

pathawks Oct 21, 2017

Member

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

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

This comment has been minimized.

Copy link
@pathawks

pathawks Oct 19, 2017

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

This comment has been minimized.

Copy link
Member

ashmaroli commented Oct 19, 2017

@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..

@DirtyF
DirtyF approved these changes Oct 19, 2017
@pathawks

This comment has been minimized.

Copy link
Member

pathawks commented Oct 19, 2017

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

This comment has been minimized.

Copy link
Member

pathawks commented Oct 19, 2017

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit e7f1ce2 into master Oct 19, 2017
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jekyllbot jekyllbot deleted the pull/rubocop branch Oct 19, 2017
jekyllbot added a commit that referenced this pull request Oct 19, 2017
@ashmaroli

This comment has been minimized.

Copy link
Member

ashmaroli commented Oct 20, 2017

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

This comment has been minimized.

Copy link
Member

pathawks commented Oct 20, 2017

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

This comment has been minimized.

Copy link
Member

DirtyF commented Oct 20, 2017

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

This comment has been minimized.

Copy link
Member

pathawks commented Oct 20, 2017

@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

This comment has been minimized.

Copy link
Member

pathawks commented Oct 20, 2017

@pathawks

This comment has been minimized.

Copy link
Member

pathawks commented Oct 20, 2017

Tada 🎉

@pathawks

This comment has been minimized.

Copy link
Member

pathawks commented Oct 20, 2017

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

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

This comment has been minimized.

Copy link
Member

DirtyF commented Oct 20, 2017

@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

This comment has been minimized.

Copy link
Member

parkr commented Oct 20, 2017

Made some notes here about how I approach the release & development process: jekyll/maintainers#2 (comment)

@pathawks

This comment has been minimized.

Copy link
Member

pathawks commented Oct 20, 2017

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

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

@DirtyF

This comment has been minimized.

Copy link
Member

DirtyF commented Oct 20, 2017

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

@pathawks

This comment has been minimized.

Copy link
Member

pathawks commented Oct 20, 2017

@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.

@pathawks pathawks mentioned this pull request Oct 20, 2017
@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
5 participants
You can’t perform that action at this time.