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 use v0.50.x
#6368
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking the time to do this 🍻👍
@@ -86,7 +86,7 @@ def fsnotify_buggy?(_site) | |||
def urls_only_differ_by_case(site) | |||
urls_only_differ_by_case = false | |||
urls = case_insensitive_urls(site.pages + site.docs_to_write, site.dest) | |||
urls.each do |_case_insensitive_url, real_urls| | |||
urls.each_value do |real_urls| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! One code update suggestion and some thoughts on the configuration changes. Instead of turning them off, it might be better to exclude certain files if we decide that the rule is helpful (most are).
@@ -42,12 +42,14 @@ def convert(content) | |||
end | |||
|
|||
private | |||
# rubocop:disable Performance/HashEachMethods | |||
def make_accessible(hash = @config) | |||
hash.keys.each do |key| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use each_key here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
each_key
broke our test-suite..
@@ -82,6 +86,12 @@ Metrics/ParameterLists: | |||
Max: 4 | |||
Metrics/PerceivedComplexity: | |||
Max: 8 | |||
Naming/FileName: | |||
Enabled: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What fails for this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From their docs,
This cop makes sure that Ruby source files have snake_case names. Ruby scripts (i.e. source files with a shebang in the first line) are ignored.
and flags us for the following:
- 'Gemfile'
- 'Rakefile'
- 'lib/theme_template/Gemfile'
- 'test/fixtures/test-dependency-theme/test-dependency-theme.gemspec'
- 'test/fixtures/test-theme/test-theme.gemspec'
which IMO is a bug because this cop existed as an enabled-by-default-cop
earlier known as Style/FileName
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah... that looks like a bug. Weird.
.rubocop.yml
Outdated
Naming/FileName: | ||
Enabled: false | ||
Naming/HeredocDelimiterCase: | ||
Enabled: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should enable this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay.. I'll go with enforcing uppercase
(default)
.rubocop.yml
Outdated
Naming/HeredocDelimiterCase: | ||
Enabled: false | ||
Naming/HeredocDelimiterNaming: | ||
Enabled: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What fails for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 'features/support/helpers.rb'
- 'test/test_redcarpet.rb'
- 'test/test_tags.rb'
are flagged for presence of EOS
as the delimiter
.rubocop.yml
Outdated
@@ -109,7 +119,7 @@ Style/Documentation: | |||
- !ruby/regexp /features\/.*.rb$/ | |||
Style/DoubleNegation: | |||
Enabled: false | |||
Style/FileName: | |||
Style/Encoding: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just exclude problematic files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This cop flags files that contain the following magic comment because from Ruby 2.0+, UTF-8 is the default source file encoding:
# encoding: UTF-8
So, IMO, we switch to when_needed
if we're to enable the cop -- only enforce an encoding comment if there are non-ASCII chars
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do when_needed
, then!
Lint/UnreachableCode: | ||
Severity: error | ||
Lint/UselessAccessModifier: | ||
Enabled: false | ||
Lint/Void: | ||
Enabled: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What even is this? 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This cop checks for the use of a return with a value in a context where it will be ignored.
In our case specifically, this cop (incorrectly ?) flags Site#config=
similar to the documented example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh we must return some value and it doesn't like that! We can enable it in a subsequent pass.
@@ -44,10 +44,14 @@ Layout/SpaceInsideBrackets: | |||
Enabled: false | |||
Lint/EndAlignment: | |||
Severity: error | |||
Lint/RescueWithoutErrorClass: | |||
Enabled: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oooo we should enable this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure about it though, since unnamed rescue
means rescuing StandardError by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but it's always better to rescue explicitly what you want to rescue instead of everything. We can do this in a subsequent pass.
@@ -21,7 +21,7 @@ Layout/EmptyLinesAroundAccessModifier: | |||
Layout/EmptyLinesAroundModuleBody: | |||
Enabled: false | |||
Layout/EndOfLine: | |||
EnforcedStyle: lf | |||
EnforcedStyle: native |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done we want lf here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, we do.. but if a developer on Windows were to run script/fmt
they'd unnecessarily get taxed by Rubocop. In my own experience, it has been irritating..
A properly configured Git on Windows automatically indexes files compatible for cross-platform use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@jekyllbot: merge +dev |
Fixes #6367