Resolves #1097: Add allow_nil qualifier to validate_presence_of
#1100
Conversation
There was a problem hiding this comment.
Some files could not be reviewed due to errors:
.rubocop.yml: Lint/EndAlignment has the wrong namespace - should be Layout
.rubocop.yml: Lint/EndAlignment has the wrong namespace - should be Layout .rubocop.yml: Lint/DefEndAlignment has the wrong namespace - should be Layout .rubocop.yml: Lint/BlockAlignment has the wrong namespace - should be Layout .rubocop.yml: Lint/ConditionPosition has the wrong namespace - should be Layout Error: The `Style/TrailingCommaInLiteral` cop no longer exists. Please use `Style/TrailingCommaInArrayLiteral` and/or `Style/TrailingCommaInHashLiteral` instead. (obsolete configuration found in .rubocop.yml, please update it)
| @@ -146,12 +180,14 @@ def disallows_and_double_checks_value_of!(value, message) | |||
| end | |||
|
|
|||
| def disallows_original_or_typecast_value?(value, message) | |||
| disallows_value_of(blank_value, @expected_message) | |||
| end | ||
|
|
||
| def blank_value | ||
| if collection? | ||
| [] | ||
| elsif expects_to_allow_nil? | ||
| '' |
There was a problem hiding this comment.
Maybe need a better implementation?
There was a problem hiding this comment.
Now that I think about it, perhaps the matcher should work like this?
- If the matcher is qualified with
allow_nil, then it should test that empty-string and nil are disallowed. - However, if it is qualified with
allow_nil, then it should only test that empty-string is disallowed (because it tests that nil is allowed).
What do you think about that?
There was a problem hiding this comment.
Perhaps what you mean is, when the matcher is not qualified with allow_nil it should disallow both '' and nil?
There was a problem hiding this comment.
IMO, both a blank string and a nil value return true to #blank?,
so as long as the ActiveModel::Validations::PresenceValidator uses #blank? to check the validity of an attribute, both a true blank string and a nil can be considered a blank string (when the attribute's type is either String or Nil).
There was a problem hiding this comment.
Perhaps what you mean is, when the matcher is not qualified with allow_nil it should disallow both
''andnil?
Yes, that's what I meant :)
IMO, both a blank string and a nil value return true to #blank?
Right, exactly. As it stands, the presence matcher only uses nil in making its assertion, and in your changes here you conditionally flip that to an empty string, but really, in the default case where allow_nil is not specified, the presence matcher needs to make use of both, since PresenceValidator is not just testing that the attribute is nil but is testing that it is blank.
There was a problem hiding this comment.
That's good! Perhaps instead of reduce you could use all? And I don't think you have to write any tests to cover this -- we are changing how the matcher works internally, and as long as these changes still make the current tests pass then you should be good.
There was a problem hiding this comment.
I have tried to use the code similar to the previous example to see if it still passes the tests.
However, after I ran the specs, there is one failure. And I think the cause might be testing agains an empty string when not qualified with allow_nil.
So I checked out c0960bd7 (the commit right before this PR), and made the following modification only:
def blank_value
if collection?
[]
else
- nil
+ ''
end
endAfter I ran the specs again, there are several failed specs expecting an assertion to fail with a message but the assertion did not fail.
So I think maybe we should not explicitly disallow an empty string when not using allow_nil.
There was a problem hiding this comment.
Also, there are several places that have an example similar to this:
it_supports(
'ignoring_interference_by_writer',
tests: {
accept_if_qualified_but_changing_value_does_not_interfere: {
changing_values_with: :nil_to_blank
},
reject_if_qualified_but_changing_value_interferes: {
model_name: 'Example',
attribute_name: :attr,
changing_values_with: :never_falsy,
expected_message: <a message>
}
}
)Though I don't fully understand how this works, I think this might already have covered the case where setting an attribute to an empty string: changing_values_with: :nil_to_blank .
There was a problem hiding this comment.
Can you go ahead and push up your modifications so that I can see the test failures? That way I can debug them for you.
Also in response to your last comment, that test is making sure that if you have an attribute that changes nil to an empty string when it is assigned, that this won't cause the presence validation to fail. I guess it's true that this is implicitly testing the empty string case, but I think it would be more clear to have an explicit test case / context so that it would be easier to find if we have to do so later.
There was a problem hiding this comment.
Some files could not be reviewed due to errors:
.rubocop.yml: Lint/EndAlignment has the wrong namespace - should be Layout
.rubocop.yml: Lint/EndAlignment has the wrong namespace - should be Layout .rubocop.yml: Lint/DefEndAlignment has the wrong namespace - should be Layout .rubocop.yml: Lint/BlockAlignment has the wrong namespace - should be Layout .rubocop.yml: Lint/ConditionPosition has the wrong namespace - should be Layout Error: The `Style/TrailingCommaInLiteral` cop no longer exists. Please use `Style/TrailingCommaInArrayLiteral` and/or `Style/TrailingCommaInHashLiteral` instead. (obsolete configuration found in .rubocop.yml, please update it)
|
This looks great. The only issue with this is that it looks like we are running into #904 on one of the tests. This isn't your fault, but it does mean that I would have to get a fix for that into master before merging this so that CI can run cleanly. Sorry :( Hopefully I can have this finished soon. |
|
Sorry it took me so long to merge this, but I took your ideas here and ran with them: 834d8d0. Thanks so much! |
Please refer to #1097.
Also related: #683.