Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improve boolean validator #24294

Merged
merged 4 commits into from
Jun 8, 2019
Merged

Improve boolean validator #24294

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b07ebe4
Improve boolean validator
Swamp-Ig Jun 4, 2019
4eb567b
Remove extra throw
Swamp-Ig Jun 5, 2019
0e9b0a8
Remove None test as discussed
Swamp-Ig Jun 7, 2019
47c79f3
Fix for tests depending on None == False
Swamp-Ig Jun 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions homeassistant/helpers/config_validation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from datetime import (timedelta, datetime as datetime_sys,
time as time_sys, date as date_sys)
from socket import _GLOBAL_DEFAULT_TIMEOUT
from numbers import Number
from typing import Any, Union, TypeVar, Callable, Sequence, Dict, Optional
from urllib.parse import urlparse
from uuid import UUID
Expand Down Expand Up @@ -81,14 +82,19 @@ def validate(obj: Dict) -> Dict:

def boolean(value: Any) -> bool:
"""Validate and coerce a boolean value."""
if value is None:
return False
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we allow this? I think this could be quite unintuitive.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the face of it I agree, however I am concerned about regressions if it's not there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't fear too many regressions, it is difficult to specify None in YAML, so I doubt many people do. (famous last words!)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest we remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will remove it and see what happens 🙈

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise just merge it 😨

if isinstance(value, bool):
return value
if isinstance(value, str):
value = value.lower()
value = value.lower().strip()
if value in ('1', 'true', 'yes', 'on', 'enable'):
return True
if value in ('0', 'false', 'no', 'off', 'disable'):
return False
raise vol.Invalid('invalid boolean value {}'.format(value))
return bool(value)
elif isinstance(value, Number):
return value != 0
raise vol.Invalid('invalid boolean value {}'.format(value))


def isdevice(value):
Expand Down
9 changes: 6 additions & 3 deletions tests/helpers/test_config_validation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,17 @@ def test_boolean():
"""Test boolean validation."""
schema = vol.Schema(cv.boolean)

for value in ('T', 'negative', 'lock'):
for value in (
'T', 'negative', 'lock', 'tr ue',
[], [1, 2], {'one': 'two'}, test_boolean):
with pytest.raises(vol.MultipleInvalid):
schema(value)

for value in ('true', 'On', '1', 'YES', 'enable', 1, True):
for value in ('true', 'On', '1', 'YES', ' true ',
'enable', 1, 50, True, 0.1):
assert schema(value)

for value in ('false', 'Off', '0', 'NO', 'disable', 0, False):
for value in (None, 'false', 'Off', '0', 'NO', 'disable', 0, False):
assert not schema(value)


Expand Down