Implement RSpec/IteratedExpectation cop. Fixes #278 #340
Conversation
|
Autocorrect could be quite complicated, so I'll leave it for now. Tested for false positives on rubocop and one more base |
spec/rubocop/cop/rspec/each_spec.rb
Outdated
| expect_no_violations(<<-RUBY) | ||
| it 'validates users' do | ||
| [user1, user2, user3].each do |user| | ||
| allow(Somethig).to receive(:method).and_return(user) |
spec/rubocop/cop/rspec/each_spec.rb
Outdated
| it 'ignores `each` with proc' do | ||
| expect_no_violations(<<-RUBY) | ||
| it 'validates users' do | ||
| [user1, user2, user3].each(&:to_s) |
There was a problem hiding this comment.
Since this proc also doesn’t set an expectation, isn’t this case covered in the following spec (line 30)?
spec/rubocop/cop/rspec/each_spec.rb
Outdated
| it 'flags `each` when expectation calls method with arguments' do | ||
| expect_violation(<<-RUBY) | ||
| it 'validates users' do | ||
| [user1, user2, user3].each { |user| expect(user).to be_an(User) } |
lib/rubocop/cop/rspec/each.rb
Outdated
| # it 'validates users' do | ||
| # expect([user1, user2, user3]).to all(be_valid) | ||
| # end | ||
| class Each < Cop |
There was a problem hiding this comment.
I agree that Each is too generic a name. IteratedExpectation is the best I can think of right now, and I don’t really like that either. Perhaps @backus can think of a better name?
There was a problem hiding this comment.
Maybe AllMatcher? I usually like looking at the rspec internal naming but it isn't super helpful in this case heh https://github.com/rspec/rspec-expectations/blob/82ab90b513dfcd0b52064f56be4b2c6218adeded/lib/rspec/matchers.rb#L638-L655
There was a problem hiding this comment.
I think IteratedExpectation is actually a pretty reasonable name. Since we may possibly have cops in the future relating to the all matcher, AllMatcher may be too general also.
There was a problem hiding this comment.
If our intention is to have multiple cops that recommend the all(...) matched then I agree. That also seems pretty reasonable given that it can have pretty complex usage
There was a problem hiding this comment.
Yeah, i just think it's definitely possible if not likely. There's already a few cops (for instance the ItBehavesLike cop i just added...) that cover lots of potential recommendations (who's to say we won't have more corrections for using it_behaves_like in the future) so I think we should start being more careful about how we name new cops.
At the risk of going slightly off topic, there's also #232 to consider at some point. Idk if I'm sold on it exactly, but another area where this was slightly awkward recently was adding a new feature to the ExampleWording cop. It didn't make sense to add a second wording cop but the two features inside there now would probably be in separate cops and individually configurable, in an ideal world.
lib/rubocop/cop/rspec/each.rb
Outdated
| def on_block(node) | ||
| each?(node) do |arg, body| | ||
| if single_expectation?(body, arg) || only_expectations?(body, arg) | ||
| @arg = arg |
There was a problem hiding this comment.
What does setting @arg do?
There was a problem hiding this comment.
left over from some initial attempts to implement autocorrect. Its not needed, as arg can be retrieved from the node itself, but I have dropped the autocorrect support completely. I'll clean up
|
PR updated |
Darhazer
changed the title
|
Looks great! Thanks @Darhazer |
Implement RSpec/IteratedExpectation cop. Fixes rubocop#278
@backus
Please look at the examples and let me know if you can think of other examples that we can detect, or some possible false positives.
TODO:
allwith anand