-
-
You must be signed in to change notification settings -
Support splitting of input_boolean config #3564
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
Conversation
for object_id, cfg in config[DOMAIN].items(): | ||
if not cfg: | ||
cfg = {} | ||
for _, p_config in config_per_platform(config, DOMAIN): |
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.
Instead of config_per_platform
, use extract_domain_configs
(also in helpers package)
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.
fixed
9218ef4
to
8d2eb4c
Compare
How do you handle duplicate entitie_ids? |
@kellerza Solved with generate_entity_id |
@balloob Any more comments on this? |
I just realised that now we a) enforce that the DOMAIN is at least present and b) we only validate the config under DOMAIN key. And so the problem with this is that we are going to get into weird territory, as this component is diverging from the usual config path: a component either has all config under it's DOMAIN or it supports multiple entries but it is also platform based. This component fits neither structure now, hence we cannot leverage the automatic config validation. We could hack around this by changing our CONFIG_SCHEMA to be a PLATFORM_SCHEMA. However this means that our config will now come in as a list of dictionaries under DOMAIN, ie: {
input_boolean: [
{ 'notify_home': { 'initial': True },
{ 'bla_di_bla': { 'initial': True },
]
} So to continue with this PR:
PLATFORM_SCHEMA = vol.Schema({
cv.slug: vol.Any({
vol.Optional(CONF_NAME): cv.string,
vol.Optional(CONF_INITIAL, default=False): cv.boolean,
vol.Optional(CONF_ICON): cv.icon,
}, None)})
|
That is exactly what this commit did for the input_* components It also raised a warning on duplicates, which is probably better than generating unique IDs, since input_* entities will always be used in automations and creating a random name will only hide config errors |
Oh @kellerza you're so right. My bad. I'm btw still not convinced that the value we gain is more than the complexity we add. |
That is exactly what swayed me not to push the argument in #3256 - it does become a bit complex 😄 |
I'm closing this as it's not worth the possible confusion for the user. |
Description:
Allow splitting of input_boolean config.
To day the example config only loads the input_boolean.test1 component, ant the input_boolean.test2 is not loaded. There is no error message in the log to tell that something is wrong.
I will do the same with input_select and input_slider, if this solution is accepted.
Related issue (if applicable): fixes #3065
Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#
Example entry for
configuration.yaml
(if applicable):Checklist:
the code does not interact with devices:
tox
run successfully. Your PR cannot be merged unless tests pass