Add new RSpec/ChangeByZero cop
#1265
Conversation
There was a problem hiding this comment.
Thank you for another contribution!
A few notes below.
Additionally, I've ran the cop against https://github.com/pirj/real-world-rspec, and it errored out on a couple of files:
An error occurred while RSpec/ChangeByZero cop was inspecting /Users/pirj/source/real-world-rspec/canvas-lms/spec/models/content_export_spec.rb:325:30.
undefined method `end_pos' for nil:NilClass
/Users/pirj/source/rubocop-rspec/lib/rubocop/cop/rspec/change_by_zero.rb:67:in `autocorrect'
expect { @ce.save! }.to change(DelayedMessage, :count).by 0
Checked a few cases - seem to be related to by with no parentheses.
And this confirms that this weird by(0) pattern is really used in the wild 🎉
ydah
force-pushed
the
add_change_by_zero
branch
3 times, most recently
from
3b990bb to
146a772
Compare
|
I updated this PR. Thank you so much! |
ydah
force-pushed
the
add_change_by_zero
branch
from
146a772 to
f29791a
Compare
| PATTERN | ||
|
|
||
| def on_send(node) | ||
| expect_change_with_arguments(node) do |
There was a problem hiding this comment.
minor: the cop will be faster if we RESTRICT_ON_SEND to change and then check the pattern against node.parent (as we will be getting the inner send node)
There was a problem hiding this comment.
the cop will be faster if we RESTRICT_ON_SEND to […]
Can we write a cop to verify the presence of RESTRICT_ON_SEND when on_send is defined?
There was a problem hiding this comment.
Thank you so much. I'll define RESTRICT_ON_SEND to change.
There was a problem hiding this comment.
Can we write a cop to verify the presence of RESTRICT_ON_SEND when on_send is defined?
I tried to create a PR rubocop/rubocop#10783 but it is now closed.
The reason is that there was a judgment that there may be more noise than detection efficiency.
Resolves: rubocop#1100 This cop enforces the use of `not_to change` over `to change.by(0)` ## example ```ruby # bad expect { run }.to change(Foo, :bar).by(0) expect { run }.to change { Foo.bar }.by(0) # good expect { run }.not_to change(Foo, :bar) expect { run }.not_to change { Foo.bar } ```
ydah
force-pushed
the
add_change_by_zero
branch
from
f29791a to
7645439
Compare
|
I add RESTRICT_ON_SEND to |
This cop will make sure that `RESTRICT_ON_SEND` is defined if `on_send` or `after_send` is defined. This comment rubocop/rubocop-rspec#1265 (comment) is also mentioned in and leads to optimize the cop by `RESTRICT_ON_SEND` whenever possible. However, this PR shall not be used to resolve to the existing cop. If we included that much in this PR, the scale would be enormous, and we thought it would be better to look at how to optimize each cop in detail.
This cop will make sure that RESTRICT_ON_SEND is defined if on_send or after_send is defined.
```ruby
# bad
class FooCop
def on_send(node)
# ...
end
end
# good
class FooCop
RESTRICT_ON_SEND = %i[bad_method].freeze
def on_send(node)
# ...
end
end
```
This comment rubocop/rubocop-rspec#1265 (comment)
is also mentioned in and leads to optimize the cop by RESTRICT_ON_SEND whenever possible.
However, this PR does not resolve existing cop.
If we included that much in this PR, the scale would be enormous,
and we thought it would be better to look at how to optimize each cop in detail.
Resolves: #1100
This cop enforces the use of
not_to changeoverto change.by(0)example
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).If you have created a new cop:
config/default.yml.Enabled: pendinginconfig/default.yml.VersionAddedindefault/config.ymlto the next minor version.If you have modified an existing cop's configuration options:
VersionChangedinconfig/default.ymlto the next major version.