-
-
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 #7549] Add Style/ArgumentsForwarding
cop
#7646
Conversation
|
7d4daa2
to
5f83341
Compare
Style/ArgumentsForwarding
copStyle/ArgumentsForwarding
cop
This PR is WIP because it has some false positives. |
config/default.yml
Outdated
@@ -2202,6 +2202,12 @@ Style/AndOr: | |||
- always | |||
- conditionals | |||
|
|||
Style/ArgumentsForwarding: | |||
Description: 'Use arguments forwarding.' | |||
Enabled: true |
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.
Going forward new cops will be introduced with status pending
.
We'll also need a matching style guide entry for this cop. |
manual/cops_style.md
Outdated
# but it will change the behavior. Because `...` forwards block also. | ||
def foo(*args) | ||
bar(*args) | ||
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.
I'm wondering if the "Strict" option is confusing.
I meant when Strict: true
, the cop adds offenses if it does not change behavior. When Strict : false
, the cop adds an offense even if it may changes the behavior.
But it looks opposite to the implementation.
# I meant the cop does not add an offense to the following code, if `Strict: true`.
# But the implementation and documentation say the opposite.
def foo(*args)
bar(*args)
end
So, I guess we can invert the meaning, change the option name, or do not change anything.
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 the option name to AllowOnlyRestArgument
.
I guess it is a good idea to add a test case that delegating block argument is not replaced with Perhaps This implementation does not treat references of arguments in other places. # This `args` should not be replaced with `...`, but it looks replacing.
def foo(*args)
args2 = args
p args2
bar(*args)
end |
Follow up of rubocop#7646 (comment). This PR accepts pending cops in the development build. Some tests fail with the following warnings when using the `Enabled: pending` status. ```console % bundle exec rake spec (snip) 1) RuboCop::CLI --only when one cop is given accepts cop names from plugins Failure/Error: raise output RuntimeError: The following cops were added to RuboCop, but are not configured. Please set Enabled to either `true` or `false` in your `.rubocop.yml` file: - Style/ZeroLengthPredicate Inspecting 2 files .. 2 files inspected, no offenses detected # ./spec/rubocop/cli/cli_options_spec.rb:213:in `block (4 levels) in <top (required)>' # ./spec/support/cli_spec_behavior.rb:26:in `block (2 levels) in <top (required)>' # ./lib/rubocop/rspec/shared_contexts.rb:29:in `block (4 levels) in <top (required)>' # ./lib/rubocop/path_util.rb:67:in `chdir' # ./lib/rubocop/path_util.rb:67:in `chdir' # ./lib/rubocop/rspec/shared_contexts.rb:28:in `block (3 levels) in <top (required)>' # ./lib/rubocop/rspec/shared_contexts.rb:8:in `block (2 levels) in <top (required)>' ``` This PR prevents those tests from catching a pending cop warning. OTOH, this PR supplements the E2E test for a pending cop warning.
Follow up of #7646 (comment). This PR accepts pending cops in the development build. Some tests fail with the following warnings when using the `Enabled: pending` status. ```console % bundle exec rake spec (snip) 1) RuboCop::CLI --only when one cop is given accepts cop names from plugins Failure/Error: raise output RuntimeError: The following cops were added to RuboCop, but are not configured. Please set Enabled to either `true` or `false` in your `.rubocop.yml` file: - Style/ZeroLengthPredicate Inspecting 2 files .. 2 files inspected, no offenses detected # ./spec/rubocop/cli/cli_options_spec.rb:213:in `block (4 levels) in <top (required)>' # ./spec/support/cli_spec_behavior.rb:26:in `block (2 levels) in <top (required)>' # ./lib/rubocop/rspec/shared_contexts.rb:29:in `block (4 levels) in <top (required)>' # ./lib/rubocop/path_util.rb:67:in `chdir' # ./lib/rubocop/path_util.rb:67:in `chdir' # ./lib/rubocop/rspec/shared_contexts.rb:28:in `block (3 levels) in <top (required)>' # ./lib/rubocop/rspec/shared_contexts.rb:8:in `block (2 levels) in <top (required)>' ``` This PR prevents those tests from catching a pending cop warning. OTOH, this PR supplements the E2E test for a pending cop warning.
@koic How are things looking here? |
The progress in this PR is slow because I have other priorities for RuboCop 1.0. I will close once if things don't work for a while. Thank you for the mention. |
82e42c6
to
5f1b7e2
Compare
Follow rubocop/rubocop#7646. This PR adds new "Arguments Forwarding" rule that prefers Ruby 2.7's arguments forwarding syntax. ```ruby # bad def some_method(*args, &block) other_method(*args, &block) end # bad def some_method(*args, **kwargs, &block) other_method(*args, **kwargs, &block) end # bad # Please note that it can cause unexpected incompatible behavior # because `...` forwards block also. # rubocop/rubocop#7549 def some_method(*args) other_method(*args) end # good def some_method(...) other_method(...) end ```
Style/ArgumentsForwarding
copStyle/ArgumentsForwarding
cop
Yeah, I opened rubocop/ruby-style-guide#845.
I added the test cases. |
Follow rubocop/rubocop#7646. This PR adds new "Arguments Forwarding" rule that prefers Ruby 2.7's arguments forwarding syntax. ```ruby # bad def some_method(*args, &block) other_method(*args, &block) end # bad def some_method(*args, **kwargs, &block) other_method(*args, **kwargs, &block) end # bad # Please note that it can cause unexpected incompatible behavior # because `...` forwards block also. # rubocop/rubocop#7549 def some_method(*args) other_method(*args) end # good def some_method(...) other_method(...) end ```
5f1b7e2
to
9376f98
Compare
Closes rubocop#7549. This PR adds `Style/ArgumentsForwarding` cop. As shown at rubocop#7549, the following code is unsafe to be arguments forwarding. ```ruby def foo(*args) bar(*args) end ``` So by setting `AllowOnlyRestArgument` configuration option to true by default, This PR makes the cop safe by default.
9376f98
to
5f75bac
Compare
Closes #7549.
This PR adds
Style/ArgumentsForwarding
cop.As shown at #7549, The following code is unsafe to be arguments forwarding.
So by setting
AllowOnlyRestArgument
configuration option to true by default, This PR makes the cop safe by default.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 RuboCop for itself, and generates the documentation.