Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add eql/equal support to RSpec/PredicateMatcher #2032

Merged
merged 1 commit into from
Feb 10, 2025

Conversation

bquorning
Copy link
Collaborator

The be_bool? node matcher was already finding nodes using eql and equal, but then didn't properly handle them.


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.
  • Updated documentation.
  • Added an entry to the CHANGELOG.md 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 bquorning requested a review from a team as a code owner February 9, 2025 12:53
@bquorning bquorning force-pushed the predicate-matcher-eql-equal branch from ab5f824 to ec1e83b Compare February 9, 2025 12:57
@bquorning
Copy link
Collaborator Author

cc @corsonknowles

Copy link
Contributor

@corsonknowles corsonknowles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great find! Another win for coverage.

@bquorning bquorning requested a review from pirj February 9, 2025 21:18
Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@@ -106,7 +106,7 @@ def rewrite_matcher(corrector, predicate, matcher)

def true?(to_symbol, matcher)
result = case matcher.method_name
when :be, :eq
when :be, :eq, :eql, :equal
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few aliases to make this complete? https://github.com/rspec/rspec/blob/a80119c703d38f993c414490abe8c992154cf5d1/rspec-expectations/lib/rspec/matchers.rb#L578

I just went there just in case, I don't think I've seen those used once in my practice 🙈 So really up to you.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I have never seen those used either. I think there’s no need to add them now.

The `be_bool?` node matcher was already finding nodes using `eql` and
`equal`, but then didn't properly handle them.
@bquorning bquorning force-pushed the predicate-matcher-eql-equal branch from ec1e83b to 49b9b21 Compare February 10, 2025 10:13
@bquorning bquorning merged commit 3d46503 into master Feb 10, 2025
27 checks passed
@bquorning bquorning deleted the predicate-matcher-eql-equal branch February 10, 2025 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants