-
-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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
Additional validators #23293
Additional validators #23293
Conversation
raise vol.Invalid('invalid boolean value {}'.format(value)) | ||
|
||
|
||
def boolean_true(value: Any) -> bool: |
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 the changes to the boolean validator are good, but I don't see what the use case of this is ?
I also think that if we want to implement it, we should generalize it. Like with_invalid_value(boolean, False)
, that will return a value if the passed in validator raises Invalid. However, I think that we're missing the point of config validation in that case, we shouldn't accept any value.
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.
Makes sense.
The motivation use case was in wait conditions in templates where we really should be using the validator, rather than just use an ad-hock conception of true that didn't align with this one. I didn't want to be logging a pile of errors though so felt making it tolerate other stuff was sensible.
I can't think of another situation where invalid output is ignored apart from the boolean case, hence not doing a with_invalid
. Maybe rename to boolean_tolerant
(with the implication that the other one is boolean_strict
?
All of these are different to the Boolean validator in vol, that one goes for the Python sense of truth, which I don't think is really what we're looking for.
def whitespace(value: Any) -> str: | ||
"""Validate result contains only whitespace.""" | ||
if isinstance(value, str) and _WS.fullmatch(value): | ||
return '' |
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.
This is not really a validation, is it 😉
_WS = re.compile('\\s*') | ||
|
||
|
||
def whitespace(value: Any) -> str: |
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.
This should really be called strip_whitespace
, but I first want to see a use case and an application of this. We cant just add validators that seem useful, without using them.
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.
Use case is really using with the Or validation, for None or some kind of object.
vol.Or(cv.whitespace, vol.integer)
Maybe return None instead, and call it whitespace_none
.
Upstream the use case is this: I am validating the output from templates, and the icon template can output whitespace or a valid icon like 'mdi:light'. Without using this first, the icon validator outputs an error (which is correct).
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.
but where in the code is it used? Because as far as I know, we don't run output of templates through validators? Instead, we run config through it.
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.
Because as far as I know, we don't run output of templates through validators?
We will soon 😁. I'm planning to (optionally) validate the rendered outputs of all the templates in the template component. Then you can log errors on invalid output instead of the whatever it is rendering into end up in an unpredictable state.
The motivation for this change was seeing that the template condition used a different conception of what True was than the config validator, which them lead me to think: what is true should really be consistent between all parts of the system, and to using the config validator directly because it basically does what we're looking for.
The next step was to see that the results of icon templates weren't being validated, and it then made sense to validate that output too. In order to make this work, the whitespace validator would need to accept whitespace. That lead me to the general situation that whitespace is sometimes treated specially, and to add the whitespace validator instead and use it in an Or.
This stuff is all upstream from here, but I've been asked to break down the larger PR into smaller chunks.
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 calling it whitespace_none_or
and passing in the chained validator? That's really the main use case.
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.
Or even whitespace_as
and supply a value to use (default=None), but still use it with the vol.Or
validator.
Then you can have useful stuff like vol.Or(cv.whitespace_as([]), [cv.string])
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 just don't know if this needs to be a validator that raises invalid? And in your example you have a list or a single value. What if the list contains a whitespace value?
Have renamed |
Also changed |
raise vol.Invalid('invalid boolean value {}'.format(value)) | ||
|
||
|
||
def boolean_tolerant(value: Any) -> bool: |
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.
Can't you just define this one as
boolean_tolerant = vol.Any(boolean, lambda val: False)
That way we're not duplicating logic.
I actually think that maybe we should call it coerce_boolean
, because it's always returning a boolean?
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.
Yep that's sensible. The reason I wanted them next to each other in the source is so the concept of true was uniform and didn't drift in different locations, that's even better.
2d1daa3
to
3969589
Compare
Closed in favour of #24294 |
Description:
Adds additional validators, and improves the boolean validator:
Improved boolean validator
No longer allows non-boolean, non-number, non-string, non None types through (invalid value). Strips out surrounding white space. It seemed odd that
" true"
would be an invalid value as would"dog"
, but[56]
would not and would validate to True.Happy just to leave as it was if desired, or maybe to allow things that python considers true to be true (like non-empty sets and maps). The core of the validator is unchanged in that strings are either explicitly true or false, or invalid, it's just more robust in the face of non-string types and whitespace.
It may be worth considering 'open' to be true and 'closed' to be false as well.
Boolean_tolerant validator
Accepts true booleans, true-like strings and non-zero number types as true (everything true above). Everything else coerced to false. The difference between this and the above is that it never results in an invalid exception and things like lists and sets are explicitly false.
Whitespace validator
Coerces any combination of whitespace characters to empty string, rejects anything else.
Useful as part of an Or validator, where whitespace or some other object is accepted. Whitespace could arguably be better defined as None.
Checklist:
tox
. Your PR cannot be merged unless tests passIf the code does not interact with devices: