-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
[Fix #6699] Fix a false negative for Layout/IndentationWidth
#6792
Conversation
check_indentation(previous_modifier, member, | ||
indentation_consistency_style) | ||
end | ||
check_members_for_rails_style(members) |
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 extracted to the method because it was warned by Metrics/AbcSize
cop.
a90d8de
to
72b490f
Compare
@@ -179,6 +176,24 @@ def check_members(base, members) | |||
end | |||
end | |||
|
|||
def select_check_member(member) | |||
if member && | |||
member.children[0].is_a?(RuboCop::AST::Node) && |
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 are some cases where the first child would not be nil
or Node
? If there are such cases, we should add unit tests. If we can't think of any, this might be a bit defensive. 🙂
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 updated to the following fine approach.
#6792 (comment)
@@ -179,6 +176,24 @@ def check_members(base, members) | |||
end | |||
end | |||
|
|||
def select_check_member(member) | |||
if member && |
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.
Perhaps we can use a node pattern to clean this up, e.g.:
def_node_matcher :access_modifier?, <<-PATTERN
[(send ...) access_modifier?]
PATTERN
def select_check_member(member)
return unless member
if access_modifier?(member.children.first)
member.children.first
else
member
end
end
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, I could not think of this method. Thank you for telling me a fine approach!
72b490f
to
98a2d04
Compare
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.
🚀
Fixes rubocop#6699. This PR fixes infinite loop for `Layout/IndentationWidth` and `Layout/IndentationConsistency` when bad modifier indentation before good method definition. ```ruby # exmaple.rb class Foo private def foo end end ``` First, auto-corrected by `Layout/IndentationConsistency`. ```console % rubocop -v 0.65.0 % rubocop -a --only Layout/IndentationConsistency example.rb Inspecting 1 file C Offenses: example.rb:4:3: C: [Corrected] Layout/IndentationConsistency: Inconsistent indentation detected. def foo ... ^^^^^^^ 1 file inspected, 1 offense detected, 1 offense corrected ``` ```diff % g diff diff --git a/example.rb b/example.rb index 23d3556..974f101 100644 --- a/example.rb +++ b/example.rb @@ -1,6 +1,6 @@ class Foo private - def foo - end + def foo + end end ``` Next, auto-corrected by `Layout/IndentationWidth`. ```console % rubocop -a --only Layout/IndentationWidth example.rb Inspecting 1 file C Offenses: example.rb:4:1: C: [Corrected] Layout/IndentationWidth: Use 2 (not 6) spaces for indentation. def foo ^^^^^^ 1 file inspected, 1 offense detected, 1 offense corrected ``` This will return to the original code. ```diff % g diff diff --git a/example.rb b/example.rb index 974f101..23d3556 100644 --- a/example.rb +++ b/example.rb @@ -1,6 +1,6 @@ class Foo private - def foo - end + def foo + end end ``` That caused the infinite loop in `Layout/IndentationConsistency` and `Layout/IndentationWidth`. With this PR, `Layout/indentationWidth` cop makes aware of the bad modifier indentation before good method definition. ```console % rubocop -a --only Layout/IndentationWidth example.rb Inspecting 1 file C Offenses: example.rb:2:1: C: [Corrected] Layout/IndentationWidth: Use 2 (not 6) spaces for indentation. private ^^^^^^ 1 file inspected, 1 offense detected, 1 offense corrected ``` ```diff diff --git a/example.rb b/example.rb index 23d3556..07525ec 100644 --- a/example.rb +++ b/example.rb @@ -1,5 +1,5 @@ class Foo - private + private def foo end ``` This PR fixes the above false negative. That would be an auto-correct expected.
98a2d04
to
f5b6244
Compare
### Summary RuboCop 0.66.0 has been released. https://github.com/rubocop-hq/rubocop/releases/tag/v0.66.0 And rubocop-0-66 channel is available in Code Climate. https://github.com/codeclimate/codeclimate/releases/tag/v0.84.0 RuboCop 0.66.0 fixed the false negative to indentation for modifier. And this PR applied the auto-correction fixed by it. rubocop/rubocop#6792 In addtion, this PR is also updating the following 4 gems that RuboCop depends on. - Update Psych gem ... rubocop/rubocop#6766 - Update Parser gem to 2.6.2.0 that supports Ruby 2.5.5 and 2.6.2 ... https://github.com/whitequark/parser/blob/v2.6.2.0/CHANGELOG.md#changelog - Remove powerpack gem ... rubocop/rubocop#6806 - Update unicode-display_width gem ... rubocop/rubocop#6813
Fixes rubocop#6861. This is a regression by rubocop#6792. This PR fixes infinite loop for `Layout/IndentationWidth` cop and `Layout/AccessModifierIndentation` cop. ```ruby # exmaple.rb class Foo private def do_something end end ``` ```yaml # .rubocop.yml % cat .rubocop.yml Layout/AccessModifierIndentation: EnforcedStyle: outdent ``` First, auto-corrected by `Layout/IndentationWidth`. ```console % rubocop -v 0.66.0 % rubocop -a --only Layout/IndentationWidth Inspecting 1 file C Offenses: example.rb:2:1: C: [Corrected] Layout/IndentationWidth: Use 2 (not 0) spaces for indentation. private 1 file inspected, 1 offense detected, 1 offense corrected ``` ```diff % git diff example.rb diff --git a/example.rb b/example.rb index 063e6a5..081b80a 100644 --- a/example.rb +++ b/example.rb @@ -1,5 +1,5 @@ class Foo -private + private def do_something end ``` Next, auto-corrected by `Layout/Layout/AccessModifierIndentation` (`EnforcedStyle: outdent`). ```console % rubocop -a --only Layout/AccessModifierIndentation Inspecting 1 file C Offenses: example.rb:2:3: C: [Corrected] Layout/AccessModifierIndentation: Outdent access modifiers like private. private ^^^^^^^ 1 file inspected, 1 offense detected, 1 offense corrected ``` This will return to the original code. ```diff % git diff diff --git a/example.rb b/example.rb index 081b80a..063e6a5 100644 --- a/example.rb +++ b/example.rb @@ -1,5 +1,5 @@ class Foo - private +private def do_something end ``` That caused the infinite loop in `Layout/IndentationWidth` and `Layout/AccessModifierIndentation` (`EnforcedStyle: outdent`). With this PR, `Layout/indentationWidth` cop makes aware of `Layout/AccessModifierIndentation` cop.
Fixes #6699.
This PR fixes infinite loop for
Layout/IndentationWidth
andLayout/IndentationConsistency
when bad modifier indentation before good method definition.First, auto-corrected by
Layout/IndentationConsistency
.Next, auto-corrected by
Layout/IndentationWidth
.This will return to the original code.
That caused the infinite loop in
Layout/IndentationConsistency
andLayout/IndentationWidth
.With this PR,
Layout/indentationWidth
cop makes aware of the bad modifier indentation before good method definition.This PR fixes the above false negative. That would be an auto-correct expected.
Before submitting the PR make sure the following are checked:
[Fix #issue-number]
(if the related issue exists).master
(if not - rebase it).and description in grammatically correct, complete sentences.
bundle exec rake default
. It executes all tests and RuboCop for itself, and generates the documentation.