Fix an error for RSpec/ChangeByZero when change (...) .by (0) and change (...), concatenated with and and or
#1984
Conversation
… `change (...)`, concatenated with `and` and `or` fix: #1983
|
Hold on @rubocop/rubocop-rspec |
| @@ -118,7 +118,11 @@ def register_offense(node, change_node) | |||
| # rubocop:enable Metrics/MethodLength | |||
|
|
|||
| def compound_expectations?(node) | |||
| %i[and or & |].include?(node.parent.method_name) | |||
| if node.parent.send_type? | |||
| %i[and or & |].include?(node.parent.method_name) | |||
There was a problem hiding this comment.
This fixes it for for and and or compound aliases. We've missed that, and you’ve added specs for this. It is the gist of the fix and it’s fine.
| if node.parent.send_type? | ||
| %i[and or & |].include?(node.parent.method_name) | ||
| else | ||
| node.parent.and_type? || node.parent.or_type? |
There was a problem hiding this comment.
This is different. Remove this and no spec will fail.
It fixes the case for the and/or operator usage.
100% coverage would detected this.
Two problems here:
- This is nonsensical for RSpec, doesn’t work as it looks like and we should not add specs for this
- It is incomplete, as the cop would still break for eg inline
if change.by 0(which makes even less sense thanand)
It fixes rubocop for someone, but they instead should fix their specs to properly use RSpec.
Would you agree to revert this statement from the fix, and to disconnect this fix from the original issue? @ydah @bquorning
There was a problem hiding this comment.
You are right @pirj. This code seems to be added to handle (irrelevant) cases like to (...) and (...) (note the missing . before the and).
And as you mention in #1983, we have to choose between raising an exception or allowing what’s most likely unintended syntax. Detecting unintended syntax might be something we should add to our list of non-goals?
There was a problem hiding this comment.
They way you put it totally appeals to me. I can’t tell off the top of my head if we already had cases that fall under this definition. But we need some statement to be able to refer to when deciding how to deal with this.
I recall doing something like expect { … }.to raise(…), which was obviously not what I wanted.
fix: #1983
Before submitting the PR make sure the following are checked:
master(if not - rebase it).CHANGELOG.mdif 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).