Improve boolean validator #24294
Improve boolean validator #24294
Conversation
| @@ -81,14 +82,20 @@ def validate(obj: Dict) -> Dict: | |||
|
|
|||
| def boolean(value: Any) -> bool: | |||
| """Validate and coerce a boolean value.""" | |||
| if value is None: | |||
| return False | |||
There was a problem hiding this comment.
Should we allow this? I think this could be quite unintuitive.
There was a problem hiding this comment.
On the face of it I agree, however I am concerned about regressions if it's not there.
There was a problem hiding this comment.
I don't fear too many regressions, it is difficult to specify None in YAML, so I doubt many people do. (famous last words!)
There was a problem hiding this comment.
Will remove it and see what happens 🙈
There was a problem hiding this comment.
Yeh... it didn't work there were a few tests depending on the None behaviour, although they were validating that it didn't actually change anything but not catching a validation exception. I've patched them so they work right.
Otherwise we can roll-back to allowing None as a value.
IDK, I'm still a bit edgy about it. Open to comments.
There was a problem hiding this comment.
Otherwise just merge it 😨
|
In YAML 1.1: I would like to suggest not making it less strict on that part. So I personally do not agree on the PS: Recently we made all documentation YAML 1.2 compatible. |
|
@frenck Yes, YAML 1.2 no longer auto-converts certain text to booleans. But the reason that was done was very simple: tons of people were trying to use the given strings as text (strings), but any valid YAML parser would happily auto-convert that to a boolean, removing the original text information. However, here we know that the given option is a boolean, so why not make it aceept a bunch of types that clearly indicate the user's intent. The only reason not to that i can think of is that having multiple possible values for booleans is potentially confusing. So for example users might be confused whether all of the above have the same semantics (which they do). But on the other hand, it would also be a huge breaking change - and i think there's a lot of value for accepting data when the user's intent is very clear. Edit: breaking change would also hurt for a long time after that: Many examples on other websites (not home-assistant.io) will no longer work. |
|
I would rather educate people than handle every possible "wrong" use-case in code just to be friendly. IMHO we should stick to spec to prevent falling into edge cases like this and giving the user the false sense of doing it right. |
|
While the package is config validation, it also validates other things too like arguments to service requests, so we're not strictly talking yaml here. I don't really feel that there's all that much to be gained by not accepting Really the main thing I didn't like about the old validator was that it validated non-empty complex types like arrays as |
|
I think that strip is fine, because YAML will also automatically strip non-quoted strings:
|
* Improve boolean validator * Remove extra throw * Remove None test as discussed * Fix for tests depending on None == False
Description:
Improved the boolean validator to make it more robust. Picked out the important bits of #23293
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.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.
Checklist:
tox. Your PR cannot be merged unless tests passIf the code does not interact with devices: