Use simple lookup table for replacements in StubbedMock
#1994
Conversation
|
Hmm, I think this spec passes both before and after the change, so there is some issue here: it 'flags `are_expected`' do
expect_offense(<<~RUBY)
are_expected.to receive(:bar).and_return(:baz)
^^^^^^^^^^^^ Prefer `` over `are_expected` when configuring a response.
RUBY
end |
corsonknowles
force-pushed
the
use_simple_lookup_table_for_replacements
branch
from
bdd66fa
to
e6bbc1d
Compare
|
Okay, I pushed up a real change -- now it only flags on the methods it can correct. Thoughts? |
corsonknowles
force-pushed
the
use_simple_lookup_table_for_replacements
branch
2 times, most recently
from
487bf67
to
86d1e71
Compare
|
|
||
| it 'flags `should`', skip: 'Not implemented yet' do | ||
| expect_offense(<<~RUBY) | ||
| foo.should receive(:bar).and_return(:baz) |
There was a problem hiding this comment.
RSpec 2 legacy, deprecated a decade ago and removed in next major RSpec. We don’t want to support this.
| it 'flags `are_expected`', | ||
| skip: 'This passed before this commit. Handled like is_expected?' do | ||
| expect_offense(<<~RUBY) | ||
| are_expected.to receive(:bar).and_return(:baz) |
There was a problem hiding this comment.
Is this from rspec-its? It’s soft-maintained, and is unlikely can be named an integral part of RSpec. I’d rather documented how to add it to the co figuration if the cop was configurable.
| end | ||
|
|
||
| def replacement(method_name) | ||
| case method_name |
There was a problem hiding this comment.
SimpleCov pointed out that we don’t have any test coverage for when method_name didn’t match any of the options. And correctly so.
I also prefer the case statement, but perhaps we should add an else raise ArgumentError or KeyError or similar, and add test coverage (where relevant).
There was a problem hiding this comment.
Ha! We can actually configure expectations DSL, and add eg the mentioned are_expected, for which this cop won’t find a substitute.
But there is no substitute for eg are_expected. Can we skip correction in this case instead of throwing an argument error?
fetch would raise, and this is also an undesirable outcome, and a breaking change.
I was never a big fan of 100% coverage, but it really starts to manifest its benefits here!
There was a problem hiding this comment.
Correcting myself. Dsl conf link. are_expected, and should_* are included.
Quick suggestion: keep the case, but limit matching to just the three options we have substitutes for. Wdyt?
There was a problem hiding this comment.
Quick suggestion: keep the
case, but limit matching to just the three options we have substitutes for. Wdyt?
That is what we had before, no? Should we add an else case, and how should it respond?
There was a problem hiding this comment.
Previously I think it was returning nil, and we auto-corrected to nonsense. Now, I think, it should just raise an offence and don’t autocorrect
corsonknowles
force-pushed
the
use_simple_lookup_table_for_replacements
branch
from
86d1e71
to
cddc4db
Compare
Use simple lookup table for replacements in
StubbedMockBefore 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.Enabled: truein.rubocop.yml.VersionAdded: "<<next>>"indefault/config.yml.If you have modified an existing cop's configuration options:
VersionChanged: "<<next>>"inconfig/default.yml.