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.

Sign up for GitHub

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

Jump to bottom

Strict predicate #1196

Merged
merged 1 commit into from Sep 4, 2020
Merged

Strict predicate #1196

merged 1 commit into from Sep 4, 2020

Conversation

marcandre
Copy link
Contributor

@marcandre marcandre commented Jun 17, 2020 •
edited

This PR adds a config setting strict_predicate_matchers which makes the checks for the dynamic predicate matchers strict. Default is false but for internal specs the setting is changed to true in spec_helper.

It changes the failure messages in the non-strict case from "expected ... to return true/false, ..." to "expected ... to be truthy/falsey, ...".

It builds on #1195

@marcandre marcandre force-pushed the strict_predicate branch 3 times, most recently from 57247ea to 750ed10 Compare June 17, 2020 22:35
@JonRowe JonRowe marked this pull request as draft June 18, 2020 09:00
@JonRowe
Copy link
Member

JonRowe commented Jun 18, 2020

Converted to draft as it requires pre-requisite PRs to be approved/merged first.

@JonRowe JonRowe changed the base branch from master to main August 2, 2020 02:06
@marcandre marcandre mentioned this pull request Sep 1, 2020
Closed
@marcandre
Copy link
Contributor Author

Rebased. CI errors are due to issues in the rspec-core gem, addressed in rspec/rspec-core#2756

@marcandre marcandre marked this pull request as ready for review September 1, 2020 14:23
pirj
pirj approved these changes Sep 1, 2020
View reviewed changes
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.

Incredible! Thank you.

Changelog.md Outdated Show resolved Hide resolved
lib/rspec/expectations/configuration.rb Outdated Show resolved Hide resolved
features/built_in_matchers/predicates.feature Outdated Show resolved Hide resolved
lib/rspec/matchers/built_in/has.rb Show resolved Hide resolved
JonRowe
JonRowe requested changes Sep 2, 2020
View reviewed changes
Copy link
Member

@JonRowe JonRowe left a comment

Choose a reason for hiding this comment

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

Mostly looks good, a few nitpicks / suggestions, including a bike shed which I apologise for but I'd rather obey the rubocop rules as they stand and change them later if people want to.

features/built_in_matchers/predicates.feature Outdated Show resolved Hide resolved
lib/rspec/expectations/configuration.rb Outdated Show resolved Hide resolved
lib/rspec/expectations/configuration.rb Show resolved Hide resolved
lib/rspec/matchers/built_in/has.rb Outdated
@@ -90,8 +90,12 @@ def predicate_method_name
predicate
end

def predicate_matches?
!!predicate_result
def predicate_matches?(value = true)
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove the space around the = here, theres a rubocop rule enforcing no spaces, which I personally disagree with but its failing currently. (Ping @pirj, happy to change the rule but not in this PR).

Copy link
Member

Choose a reason for hiding this comment

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

This still needs fixing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, forgot to amend my commit. Done

@marcandre
Copy link
Contributor Author

Should be all fixed.

JonRowe
JonRowe reviewed Sep 3, 2020
View reviewed changes
lib/rspec/expectations/configuration.rb Outdated
@@ -185,6 +186,21 @@ def on_potential_false_positives=(behavior)
@on_potential_false_positives = behavior
end


Copy link
Member

Choose a reason for hiding this comment

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

This extra blank line is failing rubocop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I'm sorry about that. Fixed

JonRowe
JonRowe requested changes Sep 3, 2020
View reviewed changes
Copy link
Member

@JonRowe JonRowe left a comment

Choose a reason for hiding this comment

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

Almost there, looks like the core build is fixed (on those Rubies that don't run Rubocop) but there is still a rubocop violation that needs fixing (sorry!)

@JonRowe JonRowe merged commit e7cab36 into rspec:main Sep 4, 2020
@marcandre
Copy link
Contributor Author

Thank you @JonRowe ❤️

@pirj
Copy link
Member

pirj commented Sep 4, 2020

Thank you, @marcandre !

@JonRowe
Copy link
Member

JonRowe commented Sep 4, 2020

Yes thank you @marcandre and thank you for your patience 😂

@marcandre
Copy link
Contributor Author

Will it be possible to make a release?

@JonRowe
Copy link
Member

JonRowe commented Sep 15, 2020

I'm aiming to find some time this week to release 3.10 which would contain this.

@marcandre marcandre mentioned this pull request Oct 11, 2020
Closed
@marcandre marcandre deleted the strict_predicate branch October 13, 2020 05:13
@marcandre
Copy link
Contributor Author

@JonRowe Any hope for a release?

@JonRowe
Copy link
Member

JonRowe commented Oct 24, 2020

A release was being prepared but is currently blocked by #1221, that needs a fix before a minor can be shipped.

@JonRowe
Copy link
Member

JonRowe commented Oct 30, 2020

Released as 3.10.0

@marcandre
Copy link
Contributor Author

Awesome @JonRowe thanks for letting me know 🎉

@pirj pirj mentioned this pull request May 6, 2021
Open
@kachick kachick mentioned this pull request Jun 6, 2021
Closed
This was referenced Jul 18, 2021
Merged
Open
pirj added a commit to pirj/discourse that referenced this pull request Jul 25, 2022
@pirj
Comply to strict predicate matchers
586f10a 
See:
 - rspec/rspec-expectations#1195
 - rspec/rspec-expectations#1196
 - rspec/rspec-expectations#1277
@pirj pirj mentioned this pull request Jul 25, 2022
Merged
pirj added a commit to pirj/discourse that referenced this pull request Jul 27, 2022
@pirj
Comply to strict predicate matchers
d5443c1 
See:
 - rspec/rspec-expectations#1195
 - rspec/rspec-expectations#1196
 - rspec/rspec-expectations#1277
tgxworld pushed a commit to discourse/discourse that referenced this pull request Jul 28, 2022
@pirj
Add RSpec 4 compatibility (#17652)
493d437 
* Remove outdated option

rspec/rspec-core@0407831

* Use the non-globally exposed RSpec syntax

rspec/rspec-core#2803

* Use the non-globally exposed RSpec syntax, cont

rspec/rspec-core#2803

* Comply to strict predicate matchers

See:
 - rspec/rspec-expectations#1195
 - rspec/rspec-expectations#1196
 - rspec/rspec-expectations#1277
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JonRowe JonRowe JonRowe requested changes

@pirj pirj pirj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@marcandre @JonRowe @pirj