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.

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

Closed
wants to merge 6 commits into from

Conversation

Swamp-Ig
Copy link
Contributor

@Swamp-Ig Swamp-Ig commented Apr 21, 2019

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:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@Swamp-Ig Swamp-Ig marked this pull request as ready for review April 21, 2019 22:43
@Swamp-Ig Swamp-Ig requested a review from a team as a code owner April 21, 2019 22:43
raise vol.Invalid('invalid boolean value {}'.format(value))


def boolean_true(value: Any) -> bool:
Copy link
Member

@balloob balloob Apr 22, 2019

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.

Copy link
Contributor Author

@Swamp-Ig Swamp-Ig Apr 22, 2019

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 ''
Copy link
Member

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:
Copy link
Member

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.

Copy link
Contributor Author

@Swamp-Ig Swamp-Ig Apr 22, 2019

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).

Copy link
Member

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.

Copy link
Contributor Author

@Swamp-Ig Swamp-Ig Apr 23, 2019

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

@Swamp-Ig Swamp-Ig Apr 23, 2019

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])

Copy link
Member

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?

@Swamp-Ig
Copy link
Contributor Author

Have renamed boolean_true to boolean_tolerant because that's more clear IMHO and taken out the pylint comments.

@Swamp-Ig
Copy link
Contributor Author

Also changed whitespace into whitespace_as as proposed.

raise vol.Invalid('invalid boolean value {}'.format(value))


def boolean_tolerant(value: Any) -> bool:
Copy link
Member

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?

Copy link
Contributor Author

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.

@Swamp-Ig Swamp-Ig self-assigned this May 4, 2019
Swamp-Ig added a commit to Swamp-Ig/home-assistant that referenced this pull request May 4, 2019
@Swamp-Ig Swamp-Ig requested a review from balloob May 6, 2019 22:09
@Swamp-Ig Swamp-Ig mentioned this pull request May 6, 2019
5 tasks
@Swamp-Ig Swamp-Ig mentioned this pull request Jun 4, 2019
5 tasks
@Swamp-Ig
Copy link
Contributor Author

Swamp-Ig commented Jun 7, 2019

Closed in favour of #24294

@Swamp-Ig Swamp-Ig closed this Jun 7, 2019
@Swamp-Ig Swamp-Ig deleted the additional-validators branch August 24, 2022 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants