Export supported_features of light.isy994 #5429
Conversation
| @@ -72,7 +72,9 @@ def turn_on(self, brightness=None, **kwargs) -> None: | |||
| @property | |||
| def state_attributes(self): | |||
There was a problem hiding this comment.
This should not be overridden in the platforms. It's only for the parent class. The attributes will be collected from existing properties, so just make sure the corresponding properties return correctly.
There was a problem hiding this comment.
By default brightness is only exposed when the device is on.
I don't have isy994, and don't know why here it is exposed always.
@jbcodemonkey added it in a3db0ec
@jbcodemonkey Is it indeed required here to always expose brightness?
There was a problem hiding this comment.
@andrey-git didn't you say that you changed the frontend so that the brightness would show depending on supported features in state attributes? That attribute is always returned from state attributes, even when off.
There was a problem hiding this comment.
a3db0ec was not correctly implemented.
There was a problem hiding this comment.
@MartinHjelmare have right. On platform use https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/helpers/entity.py#L117
There was a problem hiding this comment.
device_state_attributes win: https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/helpers/entity.py#L227-L239
There was a problem hiding this comment.
Component/BaseEntity use state_attributes and platform use device_state_attributes and can overwrite stuff from state_attributes
There was a problem hiding this comment.
ISYDevice shouldn't override state attributes either. It is allowed to overwrite device state attributes. Change that and don't change this light platform. It looks as the properties here already return correctly (Edit: except state attributes) so the attributes will be collected in the state attributes in the light ABC class. Then in the entity class state attributes and device state attributes will be merged.
There was a problem hiding this comment.
I propose someone with isy944 device make the fix then. Even if the current state of events is broken I'm afraid to break it further since I can't really test it.
There was a problem hiding this comment.
I'll take a look at this now. Some history on this, the ISY component was written right around the same time that the Entity abstract classes were being cleaned up. That is why somethings are overwritten that probably shouldn't be. In fact, I don't believe that device_state_attributes existed at the time that the ISY component was originally written.
|
Thank you @andrey-git for quickly addressing the issue even though you don't have an ISY994. 🍺 Taking the comments from this PR, I have addressed the issue in a different way in PR #5433 and tested to confirm the ISY component still works. I'll go ahead and close this PR so the new one can be reviewed. |
Description:
Related issue (if applicable): fixes #5428
Checklist:
If the code does not interact with devices:
toxrun successfully. Your PR cannot be merged unless tests pass