Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Fix #7549] Add Style/ArgumentsForwarding cop #7646

Merged
merged 1 commit into from Oct 22, 2020
Merged

[Fix #7549] Add Style/ArgumentsForwarding cop #7646

merged 1 commit into from Oct 22, 2020

Conversation

koic
Copy link
Member

@koic koic commented Jan 12, 2020 •
edited

Closes #7549.

This PR adds Style/ArgumentsForwarding cop.

As shown at #7549, The following code is unsafe to be arguments forwarding.

def foo(*args)
  bar(*args)
end

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:

  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

@koic
Copy link
Member Author

koic commented Jan 12, 2020

rake documentation-checks is failing but it will be resolved by #7645.

@koic koic force-pushed the add_style_arguments_forwarding branch 2 times, most recently from 7d4daa2 to 5f83341 Compare January 12, 2020 05:12
@koic koic changed the title [Fix #7549] Add Style/ArgumentsForwarding cop [Fix #7549] [WIP] Add Style/ArgumentsForwarding cop Jan 12, 2020
@koic
Copy link
Member Author

koic commented Jan 12, 2020

This PR is WIP because it has some false positives.

bbatsov
bbatsov reviewed Jan 12, 2020
View reviewed changes
config/default.yml Outdated
@@ -2202,6 +2202,12 @@ Style/AndOr:
- always
- conditionals

Style/ArgumentsForwarding:
Description: 'Use arguments forwarding.'
Enabled: true
Copy link
Collaborator

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.

@bbatsov
Copy link
Collaborator

bbatsov commented Jan 12, 2020

We'll also need a matching style guide entry for this cop.

pocke
pocke reviewed Jan 13, 2020
View reviewed changes
manual/cops_style.md Outdated
# but it will change the behavior. Because `...` forwards block also.
def foo(*args)
bar(*args)
end
Copy link
Collaborator

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.

Copy link
Member Author

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.

@pocke
Copy link
Collaborator

pocke commented Jan 13, 2020

I guess it is a good idea to add a test case that delegating block argument is not replaced with ... by this cop. Because foo { |...| } is a syntax error. This cop should not add any offense to the code.

Perhaps This implementation does not treat references of arguments in other places.
So can you add test case(s) for it, and update the implementation to handle it?

# This `args` should not be replaced with `...`, but it looks replacing.
def foo(*args)
  args2 = args
  p args2

  bar(*args)
end

koic added a commit to koic/rubocop that referenced this pull request Jan 23, 2020
@koic
Accept pending cops in the development build
5d6e0a3 
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.
@koic koic mentioned this pull request Jan 23, 2020
Merged
8 tasks
bbatsov pushed a commit that referenced this pull request Jan 25, 2020
@koic @bbatsov
Accept pending cops in the development build
c675fa9 
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.
@bbatsov
Copy link
Collaborator

bbatsov commented Mar 21, 2020

@koic How are things looking here?

@koic
Copy link
Member Author

koic commented Mar 22, 2020

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.

@pixeltrix pixeltrix mentioned this pull request Sep 10, 2020
Merged
@koic koic force-pushed the add_style_arguments_forwarding branch 2 times, most recently from 82e42c6 to 5f1b7e2 Compare October 8, 2020 12:28
koic added a commit to koic/ruby-style-guide that referenced this pull request Oct 8, 2020
@koic
Add new "Arguments Forwarding" rule
e48d82a 
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
```
@koic koic mentioned this pull request Oct 8, 2020
Merged
@koic koic changed the title [Fix #7549] [WIP] Add Style/ArgumentsForwarding cop [Fix #7549] Add Style/ArgumentsForwarding cop Oct 8, 2020
@koic
Copy link
Member Author

koic commented Oct 8, 2020

We'll also need a matching style guide entry for this cop.

Yeah, I opened rubocop/ruby-style-guide#845.

So can you add test case(s) for it, and update the implementation to handle it?

I added the test cases.

bbatsov pushed a commit to rubocop/ruby-style-guide that referenced this pull request Oct 8, 2020
@koic @bbatsov
Add new "Arguments Forwarding" rule
4cd001f 
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
```
@koic koic force-pushed the add_style_arguments_forwarding branch from 5f1b7e2 to 9376f98 Compare October 8, 2020 15:12
@koic
[Fix rubocop#7549] Add Style/ArgumentsForwarding cop
5f75bac 
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.
@koic koic force-pushed the add_style_arguments_forwarding branch from 9376f98 to 5f75bac Compare October 22, 2020 10:51
@koic koic merged commit 26dfdf8 into rubocop:master Oct 22, 2020
@koic koic deleted the add_style_arguments_forwarding branch October 22, 2020 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pocke pocke pocke left review comments

@bbatsov bbatsov bbatsov approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cop idea: prefer argument forwarding syntax
3 participants
@koic @bbatsov @pocke