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

Config_Validation: Added check for domain list #13052

Closed
wants to merge 1 commit into from
Closed

Config_Validation: Added check for domain list #13052

wants to merge 1 commit into from

Conversation

cdce8p
Copy link
Member

@cdce8p cdce8p commented Mar 10, 2018

Description:

Added the option to entities_domain to check if entity belongs to one of a list of domains. This could be useful for a future switch.group since input_boolean and remote expose some of the same functionality.

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

If the code does not interact with devices:

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

* E.g. if 'input_boolean.demo' is either 'input_boolean' or 'remote'
@OttoWinter
Copy link
Member

OttoWinter commented Mar 10, 2018

Over in the architecture, this kind of came up already: Do we want users to be able to specify remotes, lights, ... in their switch groups? Back then the decision was that we should actively check the domain in order to not have anything break for some reason (home-assistant/architecture#13 (comment)).

I do understand that there might be a need for combining lights and switches, but I fear this will eventually make the group platforms as "bloated" as the group component has now gotten. I'm open to suggestions. Here are some options I think we could do:

  • Option #.1: Strictly only allow the exact domain of the group platform. This is the current way.
    • Very safe. Makes sure we don't accidentally break anything.
  • Option #.2: Allow all entity types for each group platform.
    • What if I try to call fan.set_speed on a light? Just "break", ignore the unsupported entity or do something in between?
    • Either way, I feel like this could end up making the group platforms being abused like the group: component was. And my original intention was to make these things as clean as possible. I'm open to suggestions though.
  • Option #.3: Only allow domains with a subset of the features of the target domain.
    • For example, we could allow lights in the switch group platform, but not the other way around.
    • Quite safe and would support quite a lot of use-cases.
    • A bit user unfriendly because we would allow lights in switch.group, but not switches in light.group

@cdce8p
Copy link
Member Author

cdce8p commented Mar 10, 2018

I personally would go with option 3, but this PR is more meant to set the fundation to allow for any of those options.

@OttoWinter
Copy link
Member

We can (and should probably) of course move the discussion in the architecture repo. I don't however think that we should do this PR without having a group platform that actually uses it yet.

@cdce8p
Copy link
Member Author

cdce8p commented Mar 10, 2018

@OttoWinter I wouldn't necessarily agree. It is way easier to review just 42 lines of code than a new platform. If we agree that we don't want to limit ourselves to Option #.1, I believe that this would serve as a good basis for new platforms. They will follow, it's just a matter of time (I unfortunately don't have at the moment), like the discussion on the initial PR regarding this: #12592

@OttoWinter
Copy link
Member

No no, I meant that we should only merge this once another group platform that uses this is created. Maybe there are some things to consider when doing option #.3; Having code around that isn't used anywhere eventually leads to bloat and sometimes the function is even forgotten about.

@cdce8p
Copy link
Member Author

cdce8p commented Mar 12, 2018

Closing this PR for now, until there is a use case for it.

@cdce8p cdce8p closed this Mar 12, 2018
@cdce8p cdce8p deleted the entities-domain branch March 24, 2018 10:46
@home-assistant home-assistant locked and limited conversation to collaborators Jul 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants