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.

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 a false positive for RSpec/PredicateMatcher when multiple arguments and not replaceable method #1554

Merged

Conversation

ydah
Copy link
Member

@ydah ydah commented Jan 13, 2023

This PR is fix a false positive for RSpec/PredicateMatcher when multiple arguments and not replaceable method.

following code:

expect(foo).to include(foo, bar)

RSpec/PredicateMatcher autocorrects it as follows:

expect(foo.include?(foo, bar)).to be(true)

However, this is a SyntaxError.

ArgumentError:
       wrong number of arguments (given 2, expected 1)

Before submitting the PR make sure the following are checked:

  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • [-] Updated documentation.
  • Added an entry to the CHANGELOG.md if the new code introduces user-observable changes.
  • The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).

@ydah ydah force-pushed the fix-false-positive-rspec-PredicateMatcher branch from d001bbb to 1339a88 Compare January 13, 2023 16:50
@tiagotex
Copy link

tiagotex commented Jan 13, 2023

I've just downloaded the latest version and am using the following config:

RSpec/PredicateMatcher:
  Strict: true
  EnforcedStyle: explicit

It now corrects all occurrences of predicate matcher include to be similar to expect(foo.include?("bar")).to be(true), but in the case of include we would want to skip the checks for this cop. Would you be open to a PR allowing users to define ignored predicates for RSpec/PredicateMatcher?

@ydah ydah closed this Jan 14, 2023
@ydah ydah force-pushed the fix-false-positive-rspec-PredicateMatcher branch from a65f125 to 9280a68 Compare January 14, 2023 01:37
@ydah ydah reopened this Jan 14, 2023
@ydah ydah force-pushed the fix-false-positive-rspec-PredicateMatcher branch from 0149e4c to 8c0fa32 Compare January 14, 2023 01:39
@ydah ydah force-pushed the fix-false-positive-rspec-PredicateMatcher branch from 8c0fa32 to 71b9fba Compare January 14, 2023 07:09
@bquorning
Copy link
Collaborator

FYI I intend to do a v2.17.1 release when this and #1555 is merged. Followed by a v2.18.0 release with the extraction of Capybara cops (#1519).

@bquorning
Copy link
Collaborator

@pirj, @Darhazer Is this PR ok to merge?

def replaceable_matcher?(matcher)
case matcher.method_name.to_s
when 'include'
matcher.arguments.count <= 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have a test case with zero arguments, do we?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the same thing. Also, which implementation of include takes zero arguments?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
matcher.arguments.count <= 1
matcher.arguments.one?

May be just this if there are no cases where it's used with no arguments. And include does not accept a block, so expect(foo).to include is nonsense and is probably worth a cop if anyone have seen this in practice, or there are offenders in real-world apps open source code.

Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @ydah !

@bquorning
Copy link
Collaborator

Actually this PR conflicts with 3 spec examples from #1555:

expect(foo)
^^^^^^^^^^^ Prefer using `include?` over `include` matcher.
.to include(bar, <<~TEXT, 'baz')
bar
TEXT
,
expect(foo)
^^^^^^^^^^^ Prefer using `include?` over `include` matcher.
.to include(bar, <<~TEXT, 'baz')
#{bar}
TEXT
and
expect(foo)
^^^^^^^^^^^ Prefer using `include?` over `include` matcher.
.to include(bar, <<~COMMAND, 'baz')
pwd
COMMAND

… multiple arguments

This PR is fix a false positive for `RSpec/PredicateMatcher` when `include` with multiple argument

following code:
```ruby
expect(foo).to include(foo, bar)
```

`RSpec/PredicateMatcher` autocorrects it as follows:
```ruby
expect(foo.include?(foo, bar)).to be(true)
```

However, this is a SyntaxError.
```
ArgumentError:
       wrong number of arguments (given 2, expected 1)
```
@ydah ydah force-pushed the fix-false-positive-rspec-PredicateMatcher branch from 76b76f3 to 5113d6f Compare January 16, 2023 04:10
@ydah
Copy link
Member Author

ydah commented Jan 16, 2023

Sorry for the late reply. I updated this PR.

@bquorning bquorning merged commit 6b1dab5 into rubocop:master Jan 16, 2023
@ydah ydah deleted the fix-false-positive-rspec-PredicateMatcher branch January 16, 2023 06:12
bquorning added a commit that referenced this pull request Jan 16, 2023
…Matcher

Fix a false positive for `RSpec/PredicateMatcher` when multiple arguments and not replaceable method
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants