Skip to content

RFC: Only use supported light properties#15484

Merged
balloob merged 2 commits intohome-assistant:devfrom
amelchio:light-only-supported-properties
Jul 18, 2018
Merged

RFC: Only use supported light properties#15484
balloob merged 2 commits intohome-assistant:devfrom
amelchio:light-only-supported-properties

Conversation

@amelchio
Copy link
Copy Markdown
Contributor

Description:

Many light platforms support multiple products with differing features. This means that they can have properties that are not actually valid for the active device and will potentially fail when accessed. An example of such an issue is #15339.

This PR changes the light component so it only reads properties that are relevant for the supported features. Thus, light platforms will not have to guard each property that could be invalid.

The change could reveal some missing features like I fixed in a few tests.

I am unsure about the change to light/mqtt.py and I would like help to avoid the pylint comment.

Related issue (if applicable): fixes #

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox.

If the code does not interact with devices:

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

@rytilahti
Copy link
Copy Markdown
Member

From a quick look (w/o testing) this looks what one should be expecting already; when the platform itself already reports its supported features, it should not have to need to guard accesses to irrelevant features 👍

@balloob
Copy link
Copy Markdown
Member

balloob commented Jul 18, 2018

I think that this is too what I would expect.

@balloob balloob merged commit e427f9e into home-assistant:dev Jul 18, 2018
@ghost ghost removed the in progress label Jul 18, 2018
@balloob balloob mentioned this pull request Aug 3, 2018
@home-assistant home-assistant locked and limited conversation to collaborators Dec 10, 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.

4 participants