-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Added a note about the usage of NotBlank #10407
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
Conversation
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.
I have a tiny comment about wording, but other than that, LGTM! 👍
reference/constraints/Email.rst
Outdated
@@ -88,6 +88,14 @@ Basic Usage | |||
} | |||
} | |||
|
|||
.. note:: | |||
|
|||
As it happens with the rest of constraints, ``null`` values and empty strings |
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.
How about As with the rest of the constraints
?
are considered valid values. Otherwise, in addition to validating this value, | ||
you would also be requiring it, making it impossible to be optional. That's | ||
why it's common to combine this constraint with | ||
:doc:`NotBlank </reference/constraints/NotBlank>`. |
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.
I thought if we should maybe reword this a bit:
As with most of the other constraints,
null
values and empty strings are considered valid values. This is to allow them to be optional values. If the values is mandatory, a common solution is to combine this constraint withNotBlank
.
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.
I think that your wording is better. Could you please create a pull request for it? Thanks a lot!
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.
here we go: #10418
Fixes #4244 and fixes #10357.