-
-
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 false positives for Style/OperatorMethodCall
with named forwarding
#13224
Conversation
@@ -0,0 +1 @@ | |||
* [#13218](https://github.com/rubocop/rubocop/pull/13218): Fix false positives for `Style/OperatorMethodCall` with named forwarding. ([@earlopain][]) |
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.
* [#13218](https://github.com/rubocop/rubocop/pull/13218): Fix false positives for `Style/OperatorMethodCall` with named forwarding. ([@earlopain][]) | |
* [#13224](https://github.com/rubocop/rubocop/pull/13224): Fix false positives for `Style/OperatorMethodCall` with named forwarding. ([@earlopain][]) |
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 really want this checked in CI, somehow
fcea24d
to
135dad1
Compare
return DISALLOWED_ARG_TYPES.include?(argument.children.first&.type) if argument.hash_type? | ||
|
||
argument.block_pass_type? && argument.source == '&' | ||
DISALLOWED_ARG_TYPES.include?(argument.type) |
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.
It seems that the duplication with DISALLOWED_ARG_TYPES.include?
below can be eliminated. However, the combination of the method name forwarding?
and the constant name DISALLOWED_ARG_TYPES
might still be a bit unclear.
def forwarding?(argument)
type = argument.hash_type? ? argument.children.first&.type : argument.type
DISALLOWED_ARG_TYPES.include?(type)
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.
Nice suggestion, I like it. Renamed things a bit, so hopefully its clearer now
135dad1
to
8ea68f0
Compare
def anonymous_forwarding?(argument) | ||
return true if argument.forwarded_args_type? || argument.forwarded_restarg_type? | ||
return true if argument.hash_type? && argument.children.first&.forwarded_kwrestarg_type? | ||
def disallow_argument?(argument) |
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.
Hm, disallow_argument?
might be a bit ambiguous. What do you think about the names invalid_syntax_argument?
method and INVALID_SYNTAX_ARG_TYPES
constant?
def disallow_argument?(argument) | |
def invalid_syntax_argument?(argument) |
Or malformed_argument?
and MALFORMED_ARG_TYPES
, etc...
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.
invalid_syntax_argument
sounds fine
These cases are syntax errors as well, in addition to anonymous forwarding
8ea68f0
to
9a97142
Compare
Issues found with this: * rubocop#13211 * rubocop#13215 * rubocop#13216 * rubocop#13219 * rubocop#13224 * rubocop#13225 I checked some repos in the rubocop org and they seem to not be affected by this change
These cases are syntax errors as well, in addition to anonymous forwarding
Before submitting the PR make sure the following are checked:
[Fix #issue-number]
(if the related issue exists).master
(if not - rebase it).bundle exec rake default
. It executes all tests and runs RuboCop on its own code.{change_type}_{change_description}.md
if the new code introduces user-observable changes. See changelog entry format for details.