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

Bump Rubocop to v0.48 #5997

Merged
merged 15 commits into from Apr 9, 2017

Conversation

Projects
None yet
6 participants
@ashmaroli
Member

ashmaroli commented Apr 1, 2017

Fixes #5992, fixes #6005
WIP

Rubocop 0.48 introduced some new cops that broke our tests. While we temporarily locked to the previous version 0.47.1, this pull will remove the lock to bump Rubocop to v0.48 and beyond.

Lets discuss if the fixes proposed here are acceptable.

The remaining catches appear to me as bugs in Rubocop-0.48

ashmaroli added some commits Mar 31, 2017

disable SymbolArray cop in `~> 0.48`
IMO an array of symbols read better when the constituent entries appear to
to be symbols (e.g. [:label, :type] instead of %i(label type))
Disable IndentHeredoc StyleCop
This cop recommends using sqiggly-heredocs (`<<~`) -- a Ruby 2.3 feature
or otherwise depend on external libraries like Activesupport, Powerpack,
or Unindent.

The better alternative is to disable this cop for as long as we support
Rubies older than v2.3

Ref: rubocop-hq/rubocop#4028
@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Apr 1, 2017

Member

All of the tests are failing

Member

pathawks commented Apr 1, 2017

All of the tests are failing

@@ -138,6 +138,8 @@ Style/StringLiterals:
EnforcedStyle: double_quotes
Style/StringLiteralsInInterpolation:
EnforcedStyle: double_quotes
Style/SymbolArray:
Enabled: false

This comment has been minimized.

@envygeeks

envygeeks Apr 1, 2017

Contributor

This is an opinion and not a fact, this needs to be shipped as a unique pull request for everyone else to comment.

@envygeeks

envygeeks Apr 1, 2017

Contributor

This is an opinion and not a fact, this needs to be shipped as a unique pull request for everyone else to comment.

This comment has been minimized.

@ashmaroli

ashmaroli Apr 1, 2017

Member

Agreed.

@ashmaroli

ashmaroli Apr 1, 2017

Member

Agreed.

This comment has been minimized.

@ashmaroli

ashmaroli Apr 1, 2017

Member

this needs to be shipped as a unique pull request for everyone else to comment.

This PR will ship with this cop disabled in line with behavior from 0.47.1 It can enabled via a dedicated PR

@ashmaroli

ashmaroli Apr 1, 2017

Member

this needs to be shipped as a unique pull request for everyone else to comment.

This PR will ship with this cop disabled in line with behavior from 0.47.1 It can enabled via a dedicated PR

@@ -106,7 +106,7 @@ def siteify_file(file, overrides_front_matter = {})
front_matter = {
"title" => title,
"permalink" => "/docs/#{slug}/",
"note" => "This file is autogenerated. Edit /#{file} instead."
"note" => "This file is autogenerated. Edit /#{file} instead.",

This comment has been minimized.

@envygeeks

envygeeks Apr 1, 2017

Contributor

This is not the Ruby way. This is the Go way.

@envygeeks

envygeeks Apr 1, 2017

Contributor

This is not the Ruby way. This is the Go way.

This comment has been minimized.

@ashmaroli

ashmaroli Apr 4, 2017

Member

We follow this style at Jekyll since last few releases of Rubocop

@ashmaroli

ashmaroli Apr 4, 2017

Member

We follow this style at Jekyll since last few releases of Rubocop

Show outdated Hide outdated lib/jekyll/utils.rb
@@ -139,7 +139,7 @@ def parse_date(input, msg = "Input could not be parsed.")
# Returns true if the YAML front matter is present.
# rubocop: disable PredicateName
def has_yaml_header?(file)
!!(File.open(file, "rb", &:readline) =~ %r!\A---\s*\r?\n!)
!File.open(file, "rb", &:readline) !~ %r!\A---\s*\r?\n!

This comment has been minimized.

@envygeeks

envygeeks Apr 1, 2017

Contributor

You still have a ! so this change does nothing but superficially confuse the user. !! is straight forward and easy to spot from the get-go, where as ! then later !~ is confusing, without a significant performance improvement, this suites nothing.

@envygeeks

envygeeks Apr 1, 2017

Contributor

You still have a ! so this change does nothing but superficially confuse the user. !! is straight forward and easy to spot from the get-go, where as ! then later !~ is confusing, without a significant performance improvement, this suites nothing.

This comment has been minimized.

@ashmaroli

ashmaroli Apr 1, 2017

Member

The changes in this commit was made by rubocop -a

@ashmaroli

ashmaroli Apr 1, 2017

Member

The changes in this commit was made by rubocop -a

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Apr 1, 2017

Member

All of the tests are failing

I'm investigating.. 😭

Member

ashmaroli commented Apr 1, 2017

All of the tests are failing

I'm investigating.. 😭

ashmaroli added some commits Apr 1, 2017

Revert "Use `!~` instead of inverting `=~`." and disable InverseMetho…
…ds cop

This reverts commit 1a00f8f and
additionally disables InverseMethods StyleCop
Test with Rubocop's `master` branch
Many bugfixes have been applied following the release of v0.48. Check if
those buggfixes apply to our failing tests.
@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Apr 2, 2017

Member

Note: Will remove the last two temporary commits as and when associated issues have been resolved elsewhere. Labelled do-not-merge till then.

Member

ashmaroli commented Apr 2, 2017

Note: Will remove the last two temporary commits as and when associated issues have been resolved elsewhere. Labelled do-not-merge till then.

Show outdated Hide outdated Gemfile
@@ -25,9 +25,9 @@ group :test do
gem "nokogiri"
gem "rspec"
gem "rspec-mocks"
gem "rubocop", "~> 0.47.1"
gem "test-theme", :path => File.expand_path("./test/fixtures/test-theme", File.dirname(__FILE__))
gem "rubocop", :git => "https://github.com/smakagon/rubocop", :branch => "4227_ambiguous_block_association_false_positive"

This comment has been minimized.

@pathawks

pathawks Apr 2, 2017

Member

Clearly we don't want to merge this.

@pathawks

pathawks Apr 2, 2017

Member

Clearly we don't want to merge this.

@DirtyF DirtyF added the do-not-merge label Apr 2, 2017

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Apr 2, 2017

Member

The checksum issue with gem faraday has been resolved upstream and consequently the temporary patch commit in this PR has been removed.

Member

ashmaroli commented Apr 2, 2017

The checksum issue with gem faraday has been resolved upstream and consequently the temporary patch commit in this PR has been removed.

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Apr 2, 2017

Member

Removed the temporary patch commit from a fork of Rubocop, which has been merged into Rubocop's master.

Member

ashmaroli commented Apr 2, 2017

Removed the temporary patch commit from a fork of Rubocop, which has been merged into Rubocop's master.

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Apr 4, 2017

Member

@pathawks locking to latest release of Rubocop v0.48.1
@DirtyF the do-not-merge label may be removed.

Member

ashmaroli commented Apr 4, 2017

@pathawks locking to latest release of Rubocop v0.48.1
@DirtyF the do-not-merge label may be removed.

Show outdated Hide outdated Gemfile
@@ -25,7 +25,7 @@ group :test do
gem "nokogiri"
gem "rspec"
gem "rspec-mocks"
gem "rubocop", :git => "https://github.com/bbatsov/rubocop"
gem "rubocop", "~> 0.48.1"

This comment has been minimized.

@envygeeks

envygeeks Apr 4, 2017

Contributor

👍

@envygeeks

envygeeks Apr 4, 2017

Contributor

👍

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Apr 6, 2017

Member

ping @pathawks requesting a re-review 😃

Member

ashmaroli commented Apr 6, 2017

ping @pathawks requesting a re-review 😃

@pathawks pathawks removed the do-not-merge label Apr 6, 2017

@parkr

parkr approved these changes Apr 6, 2017

Some comments/questions/concerns!

Style/IndentationWidth:
Severity: error
Style/InverseMethods:
Enabled: false

This comment has been minimized.

@parkr

parkr Apr 6, 2017

Member

What is this?

@parkr

parkr Apr 6, 2017

Member

What is this?

}
assert_nil(
@files.find { |doc| doc.relative_path == "_slides/non-outputted-slide.html" }
)

This comment has been minimized.

@parkr

parkr Apr 6, 2017

Member

What about

assert_nil @files.find do |doc|
  ...
end

Or does it complain about not having parentheses?

@parkr

parkr Apr 6, 2017

Member

What about

assert_nil @files.find do |doc|
  ...
end

Or does it complain about not having parentheses?

refute_nil(
@site.posts.index do |post|
post.relative_path == "_posts/2017-2-5-i-dont-like-zeroes.md"
end

This comment has been minimized.

@parkr

parkr Apr 6, 2017

Member

Maybe in all these cases what we should do is pull out the method with the block, or use a unified helper (i thought we had one?):

post_of_interest = @site.posts.find {|post| post.relative_path == "_posts/2017-2-5-i-dont-like-zeroes.md" }
refute_nil post_of_interest

or, with a helper method

refute_nil find_by(:relative_path, "_posts/2017-2-5-i-dont-like-zeroes.md", @site.posts)
@parkr

parkr Apr 6, 2017

Member

Maybe in all these cases what we should do is pull out the method with the block, or use a unified helper (i thought we had one?):

post_of_interest = @site.posts.find {|post| post.relative_path == "_posts/2017-2-5-i-dont-like-zeroes.md" }
refute_nil post_of_interest

or, with a helper method

refute_nil find_by(:relative_path, "_posts/2017-2-5-i-dont-like-zeroes.md", @site.posts)

This comment has been minimized.

@ashmaroli

ashmaroli Apr 7, 2017

Member

❤️ the helper method idea. Tackle in another PR?

@ashmaroli

ashmaroli Apr 7, 2017

Member

❤️ the helper method idea. Tackle in another PR?

@DirtyF

This comment has been minimized.

Show comment
Hide comment
@DirtyF

DirtyF Apr 9, 2017

Member

@jekyllbot: merge +dev

Member

DirtyF commented Apr 9, 2017

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit 7d7a312 into jekyll:master Apr 9, 2017

2 checks passed

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

jekyllbot added a commit that referenced this pull request Apr 9, 2017

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

Rubocop: Enable Style/SymbolArray
This was disabled in #5997 because it was new to Rubocop. I think it can
be enabled now.

@ashmaroli ashmaroli deleted the ashmaroli:rubocop048 branch Oct 23, 2017

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