Add support for built-in exist matcher

[michaelabon]

I've inherited a codebase that is full of expect(foo.exists?).to be_truthy. The RSpec/PredicateMatcher already checks for most built-ins, but doesn't know about the built-in exist matcher. As a result, it suggests I replace my code withexpect(foo).to be_exists.

I am not fully happy with the change I made in the explicit module. I'm happy to adjust that if y'all have a better idea.

[Fixes #716]

--

Before submitting the PR make sure the following are checked:

• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• Added tests.
• Added an entry to the changelog if the new code introduces user-observable changes.
• The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).

[bquorning]

Thank you for the PR @MKenyon.

I saw your bug report #716, but I am not sure if this logic belongs in PredicateMatcher. Strictly speaking, that cop should only be concerned with be_<foo> and has/have_<foo> matchers. Well, at least that’s my opinion – based on how rspec-expectations has separate logic for these matchers, as well as a separate documentation topic: https://relishapp.com/rspec/rspec-expectations/v/3-8/docs/built-in-matchers/predicate-matchers

That said, the cop already contains logic for other built-in matchers (is_a?, instance_of?, include?, respond_to?)… Perhaps we should have separate cops for each built-in matcher? That would make it easier for our users to enable/disable them, but possibly lead to a lot of almost duplicated cops…

[Darhazer]

The cop actually needs to know about other predicate matchers in order not to suggest turning them to be_ methods. Having said that, I'm OK with keeping the logic in this cop for the time being.

[Darhazer]

maybe just add exist in the BUILD_IN_MATCHERS ?

[michaelabon]

@Darhazer: Right. As I mentioned, I'm not fully happy with this line. Upon further investigation, you can see that the BUILT_IN_MATCHERS are only checked for be_ and have_ matchers.

179 name.start_with?('be_', 'have_') &&
180             !BUILT_IN_MATCHERS.include?(name) &&
181             !name.end_with?('?')

When I look at the specs, other built-in equivs such as include? and respond_to? only have specs for inflected. The only specs for when enforced style is explicit are for be_ and have_ matchers.

I would propose dropping checks for exist? from the when enforced style is explicit context. Alternatively (and likely in another PR), we could add explicit style support for all these built-ins.

[Darhazer]

how about:

def predicate_matcher_name?(name)
  name = name.to_s
  return false if BUILT_IN_MATCHERS.include?(name)
  name.start_with?('be_', 'have_') && !name.end_with?('?')
end

[michaelabon]

Yep, that'll work.

[Darhazer]

Oh, I've been mistaken. It will not work, as it have to return true for exist. Seems that we'll need another array for the build-in matchers that should not be ignored.

I see you just removed it, so with explicit it will not require replacing to exist with .exist?).to be_truthy. I'm fine with leaving this out of scope and just fix the inflect style to use exist and not be_exist. We will have to rework the cop at some point in the future.

@bquorning WDYT?

[Darhazer]

On second thought, to exist can't be auto-corrected, as it could not know if the object responds to exist? or exists?, so it's even better now that exist is accepted.

[bquorning]

I’m still a bit hesitant to add more complexity into this cop. I’m considering if we could split the cop in to a few cops instead – I’ll have to look into that at some point.

But I think this change an acceptable patch to fix a real bug. 👍