Keep consistency on allow_blank and allow_nil qualifiers #747
Conversation
|
I'm seeing same spec failures in #746 too 😿 |
|
I'm not sure why this is failing off the bat. It seems like a Travis-specific error or something... |
|
I restarted the Travis build, and now we have "real" failures: https://travis-ci.org/thoughtbot/shoulda-matchers/jobs/69202224. Are those related with your PR @maurogeorge? If not, can you rebase your branch on top of latest master and see if it continues to fail? Thank you! :) |
|
The branch is already up to date with master. I wonder if master would fail on CI right now, @mcmire? |
|
Hi @tute thanks for your feedback. These failures are not related with the PR, I think it is still a Travis problem. The error is |
| @@ -302,12 +302,12 @@ def in_range(range) | |||
| self | |||
| end | |||
|
|
|||
| def allow_blank(allow_blank = true) | |||
| def allow_blank | |||
| @options[:allow_blank] = allow_blank | |||
There was a problem hiding this comment.
Do we need to change allow_blank on this line to true? It seems like this would create an infinite recursive loop. (Same for the line below)
There was a problem hiding this comment.
You are right, I updated the code.
As a note we need to add specs to allow_blank and allow_nil qualifiers of this matcher.
After some discussion[1] was decided that these qualifiers do not accept any parameter. thoughtbot#722
maurogeorge
force-pushed
the
allow_nil_allow_blank-remove-param
branch
from
dc072cd
to
f2edde6
Compare
|
Yay it passed! |
|
I'm going to hold off until merging this until 3.1.0 as this doesn't seem like a time-sensitive issue. |
|
No problem 👍 |
After some discussion was decided that these qualifiers do not accept any parameter.
Closes #721