Person schema for merge_packages #21307#21703
Conversation
homeassistant
added
cla-signed
integration: person
core
small-pr
ghost
assigned
|
|
||
| CONFIG_SCHEMA = vol.Schema({ | ||
| vol.Optional(DOMAIN): vol.Any(vol.All(cv.ensure_list, [PERSON_SCHEMA]), {}) | ||
| vol.Optional(DOMAIN): vol.All(cv.ensure_list, vol.Any([PERSON_SCHEMA], [])) |
There was a problem hiding this comment.
Would it work to accept [{}] in the schema? Then we wouldn't need to change cv.ensure_list.
There was a problem hiding this comment.
I guess it could, but will have to put in a specific check in person's setup to ignore empty lists, likely here: if not conf: continue.
There was a problem hiding this comment.
Right. You mean empty dict, right? Yeah, that's also inconvenient.
There was a problem hiding this comment.
Empty dict yes. Guess it's better than fiddling with ensure_list, which is used everywhere
kellerza
changed the title
|
|
||
| config_data = self.config_data = OrderedDict() | ||
| for conf in config_persons: | ||
| if not conf: |
There was a problem hiding this comment.
This is weird. I think that this is a flaw in packages and should not be fixed in the person component. That means that any other integration in the future will need to add checks in case packages are used, that's wrong.
There was a problem hiding this comment.
An alternative is that ensure_list should return [] for an empty config branch.
It’s supposed to be null, but with our Yaml loader we convert an empty branch to an empty OrderedDict
There was a problem hiding this comment.
Added a `remove_falsy' validator to fix this. Also rolled back changes to the tests to show it has no impact
| CONFIG_SCHEMA = vol.Schema({ | ||
| vol.Optional(DOMAIN): vol.Any(vol.All(cv.ensure_list, [PERSON_SCHEMA]), {}) | ||
| vol.Optional(DOMAIN): vol.All( | ||
| cv.ensure_list, cv.remove_falsy, vol.Any([PERSON_SCHEMA], [])) |
There was a problem hiding this comment.
I don't think we need to accept an empty list specifically. That will be allowed by default.
|
|
||
| def remove_falsy(value: Sequence[T]) -> Sequence[T]: | ||
| """Remove falsy values from a list.""" | ||
| return [v for v in value if v] |
There was a problem hiding this comment.
Do you mean unit tests or test if a dictionary?
…#21703) * Person schema for merge_packages home-assistant#21307 * empty list * skip empty persons * hound * test schema * ensure_none * remove any test changes * remove_falsy validator * nice! * coretests
|
Is it possible this PR is linked to this issue #23424 ? |
|
Before this it was not even allowed to be configured in a package. They report it stopped working, so unlikely |
8 participants
Description:
Change person schema to always be a list. Skip an empty
{}dictRelated issue (if applicable): fixes #21307
Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#<home-assistant.io PR number goes here>
Example entry for
configuration.yaml(if applicable):Checklist:
lazytoxIf the code does not interact with devices: