rubocop / rubocop-rspec

Code style checking for RSpec files.
https://docs.rubocop.org/rubocop-rspec
MIT License
805 stars 277 forks source link

RSpec/PredicateMatcher: more careful approach? #466

Open zverok opened 7 years ago

zverok commented 7 years ago

Imagine the code:

expect(parser.handles?('string')).to be_truthy

RSpec/PredicateMatcher will suggest to change this into:

expect(parser).to be_handles('string')

...which reads pretty unnatural, to say the least. Maybe there could be some check to prevent this suggestion in some of cases? (It is hard to clearly specify them, though)

backus commented 7 years ago

The ExampleWording cop (I think that is the name) already does some checks like this. I have no time to get to this stuff but if you're interested you could start looking at that code

schwern commented 4 years ago

Perhaps an option to allow explicit matchers if there's an argument, and set it to true by default.

# Good
expect(Thing.exists?(id)).to be_falsey

# Bad
expect(Thing).to exist(id)

I've found predicate matchers to be hard to understand, it adds another layer between the test and the code being tested, but once I figured out the pattern the compact nature is worth it. But with an argument it's much more difficult to understand and it defeats the "reads like English" intent.

schwern commented 4 years ago

What might make this better is a with or by method to take the arguments.

expect(Thing).to exist.with(id)
expect(obj).to be_handled.by(handler)

I don't know if it's worth the effort, it makes predicate matchers even more divorced from the code, but at least it reads better.

pirj commented 4 years ago

There's a long thread in a quite recent #919 with a lot of thoughts on predicate matchers. Unrelated to this question, but linking anyway since this relates to the same cop.

pirj commented 4 years ago

I'm not convinced enought that the latter is a proper semantic replacement for the former.

expect(parser.handles?('string')).to be_truthy
expect(parser).to be_handled.by('string')

Not to say it's not supported by RSpec itself, and if it will be, it will only happen in RSpec 4. And we don't have support for different RSpec versions (e.g. skip offence for 3.0, but raise and autocorrect for 4.0). With not many changes coming with RSpec 4.0, more likely a major cleanup, I'm not sure it's even worth it.