-
-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Change the expectation for predicate methods #2353
Conversation
I guess those two PRs will be red until any one of those PRs is merged. Unfortunately, our build system is not sophisticated enough to check out the "same branch" to test multi-repo changes. |
Should I fix it here? |
Thanks @pirj For 1, I think it could be addressed in another PR |
I don't see it used anywhere as well, but if you add this here:
it passes, same with |
|
Predicate methods typically return true or false, this is part of an associated bug-fix in rspec/rspec-core#2736 but as rspec-rails is now a separate entity we must account for this ourselves. Note some predicates are here by default settings and may be able to be tidied up in `rspec-rails` 5.
24e528b
to
c6779d6
Compare
@pirj I've taken this as base and improved it. Assuming it goes green we should merge it. My reasoning here:
|
Looks good! 👍 However, regarding backwards-compatibility. We're changing what those predicates return, don't we? Previously |
Yes which is my concern with rspec/rspec-core#2736 too but I've come to the decision this can be safely considered a bug fix, its not expected that predicate methods would be used except as truthy/falsely |
4cf25ad
to
9d5c6ee
Compare
This is green on CI, just hasn't updated Github. |
@@ -105,7 +106,35 @@ def render_views | |||
end | |||
|
|||
def render_views? | |||
rendering_views | |||
rendering_views? | |||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
previous definition of use_transactional_fixtures? was here
in rspec-core
builds. We'll have to resort to attr_accessor
.
Change the expectation for predicate methods
Predicate methods typically return true or false, and should only exist for the uncountable and discrete, e.g.
use_transactional_fixtures?
, but notmax_formatted_output_length?
.This relates to the changes made in rspec/rspec-core#2736