Use voluptuous for input_slider, input_boolean, input_select #3256
Conversation
| cv.slug: { | ||
| vol.Optional(CONF_NAME): cv.string, | ||
| vol.Optional(CONF_INITIAL, default=False): cv.boolean, | ||
| vol.Optional(CONF_ICON): cv.icon, |
There was a problem hiding this comment.
CONF_NAME and CONF_ICON are present in const.py.
| if object_id != slugify(object_id): | ||
| _LOGGER.warning("Found invalid key for boolean input: %s. " | ||
| "Use %s instead", object_id, slugify(object_id)) | ||
| for object_id, cfg, duplicate in dict_items_from_list(config[DOMAIN]): |
There was a problem hiding this comment.
What's this doing? We should never be able to get a duplicate keys because we iterate over a dictionary.
There was a problem hiding this comment.
Oh, I get it. You have added a PLATFORM_SCHEMA instead of CONFIG_SCHEMA, so bootstrap is collecting your config. Change that and you can use the original for object_id, cfg in config[DOMAIN].items():
There was a problem hiding this comment.
Multiple platform style was nice for split config though, but removing it will make it simpler
|
I think that your changes to |
|
Yeah, I must probably rebase after the async PR to fix these, but the log_error(...p_config...) is a bug copied over from that first bootstrap PR of mine. It never seemed to fire during unit testing though |
kellerza
force-pushed
the
input
branch
2 times, most recently
from
58b4bab
to
5cf973a
Compare
| try: | ||
| message += humanize_error(config, ex) | ||
| except TypeError: | ||
| # Shorten ex.path - can be caused by a vol.All(ensure_list, <fail>) |
There was a problem hiding this comment.
But what if the user did provide a list in his original config, did we just lose some path?
There was a problem hiding this comment.
Validating {Key: None} with vol.Schema({Key: vol.All(ensure_list, [str])})
The None will not be a string so the path=['Key', 0] -- the first element in the array of key
So None[0] raises TypeError. By popping 1 valie off the end of path, we get a error on Key instead of a unhandled TypeError exception
There was a problem hiding this comment.
I opened alecthomas/voluptuous#206 that will address the TypeError exception by humanize_error
| message += humanize_error(config, ex) | ||
| try: | ||
| message += humanize_error(config, ex) | ||
| except TypeError: |
There was a problem hiding this comment.
What can cause a TypeError to be raised here?
| [cv.string]), | ||
| vol.Optional(CONF_INITIAL): cv.string, | ||
| vol.Optional(CONF_ICON): cv.icon, | ||
| }, _cv_input_select)}}, required=True) |
There was a problem hiding this comment.
Please add extra=vol.ALLOW_EXTRA or else this config will not work in conjunction with other components in the config.
There was a problem hiding this comment.
Wow, unit tests completely missed this and did not do a manual test after changing from PATFORM_SCHEMA
| vol.Optional(CONF_NAME): cv.string, | ||
| vol.Optional(CONF_INITIAL, default=False): cv.boolean, | ||
| vol.Optional(CONF_ICON): cv.icon, | ||
| })}}, required=True) |
| vol.Range(min=1e-3)), | ||
| vol.Optional(CONF_ICON): cv.icon, | ||
| vol.Optional(ATTR_UNIT_OF_MEASUREMENT): cv.string | ||
| }, _cv_input_slider)}}, required=True) |
| raise vol.Invalid('Maximum ({}) is not greater than minimum ({})' | ||
| .format(minimum, maximum)) | ||
| state = cfg.get(CONF_INITIAL, minimum) | ||
| state = max(min(state, maximum), minimum) |
There was a problem hiding this comment.
For all other checks we raise if the value is invalid, but here we actually change the value. Let's raise here too
There was a problem hiding this comment.
It will be a change in current behaviour, but in the end more consistent. Will do the change
| })) | ||
| for cfg in invalid_configs: | ||
| self.assertFalse( | ||
| _setup_component(self.hass, DOMAIN, {DOMAIN: cfg})) |
There was a problem hiding this comment.
Please use setup_component. The other method is an implementation detail.
| """Wrap value in list if it is not one.""" | ||
| return value if isinstance(value, list) else [value] | ||
| return (value if isinstance(value, list) else |
There was a problem hiding this comment.
I don't like that ensure_list now has a default value ([]). I would prefer instead to keep it to the original implementation.
There was a problem hiding this comment.
Ok, will roll back this change.
Ok if I comment out the failing unit tests with [None] with a comment linking to the voluptuous PR?
Description:
#2800