-
-
Notifications
You must be signed in to change notification settings - Fork 276
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
Fix a false positive for RSpec/PredicateMatcher
when multiple arguments and not replaceable method
#1554
Conversation
d001bbb
to
1339a88
Compare
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 |
a65f125
to
9280a68
Compare
0149e4c
to
8c0fa32
Compare
8c0fa32
to
71b9fba
Compare
71b9fba
to
76b76f3
Compare
def replaceable_matcher?(matcher) | ||
case matcher.method_name.to_s | ||
when 'include' | ||
matcher.arguments.count <= 1 |
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.
We don't have a test case with zero arguments, do we?
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 thought the same thing. Also, which implementation of include takes zero arguments?
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.
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.
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.
Thank you, @ydah !
Actually this PR conflicts with 3 spec examples from #1555: rubocop-rspec/spec/rubocop/cop/rspec/predicate_matcher_spec.rb Lines 354 to 358 in 3a31ab1
rubocop-rspec/spec/rubocop/cop/rspec/predicate_matcher_spec.rb Lines 373 to 377 in 3a31ab1
rubocop-rspec/spec/rubocop/cop/rspec/predicate_matcher_spec.rb Lines 392 to 396 in 3a31ab1
|
… 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) ```
76b76f3
to
5113d6f
Compare
Sorry for the late reply. I updated this PR. |
…Matcher Fix a false positive for `RSpec/PredicateMatcher` when multiple arguments and not replaceable method
This PR is fix a false positive for
RSpec/PredicateMatcher
when multiple arguments and not replaceable method.following code:
RSpec/PredicateMatcher
autocorrects it as follows:However, this is a SyntaxError.
Before submitting the PR make sure the following are checked:
master
(if not - rebase it).CHANGELOG.md
if the new code introduces user-observable changes.bundle exec rake
) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).