Closed YogevBokobza closed 1 month ago
@thecode @TomerFi Finally, this is ready for your review.
@thecode I think we are going to have a little issue in HA with this, Right now shutter position and direction are single vars and here this is an array, Also for light in HA this is an Array (because of S11), and here this is single var.
Theoretically, this library is well-coded with those use cases, but in HA there will be a split in those use cases. What do you think?
@thecode I think we are going to have a little issue in HA with this, Right now shutter position and direction are single vars and here this is an array, Also for light in HA this is an Array (because of S11), and here this is single var.
Theoretically, this library is well-coded with those use cases, but in HA there will be a split in those use cases. What do you think?
I don't understand the problem you are describing, these are well typed: https://github.com/TomerFi/aioswitcher/blob/fca042f8a6f2eefff93561b530e80a31c220a1cb/src/aioswitcher/device/__init__.py#L348-L359
And in HA you check the type and add the correct number of entities: https://github.com/home-assistant/core/blob/dcb6c9a13375d5db73211f250a52c5342d36307d/homeassistant/components/switcher_kis/cover.py#L43-L47
So you can just add another case similar to what we do in the switch platform: https://github.com/home-assistant/core/blob/dcb6c9a13375d5db73211f250a52c5342d36307d/homeassistant/components/switcher_kis/switch.py#L70-L73
Can you explain what is the problem we are going to face in HA?
@thecode I think we are going to have a little issue in HA with this, Right now shutter position and direction are single vars and here this is an array, Also for light in HA this is an Array (because of S11), and here this is single var. Theoretically, this library is well-coded with those use cases, but in HA there will be a split in those use cases. What do you think?
I don't understand the problem you are describing, these are well typed:
And in HA you check the type and add the correct number of entities: https://github.com/home-assistant/core/blob/dcb6c9a13375d5db73211f250a52c5342d36307d/homeassistant/components/switcher_kis/cover.py#L43-L47
So you can just add another case similar to what we do in the switch platform: https://github.com/home-assistant/core/blob/dcb6c9a13375d5db73211f250a52c5342d36307d/homeassistant/components/switcher_kis/switch.py#L70-L73
Can you explain what is the problem we are going to face in HA?
This for example: https://github.com/home-assistant/core/blob/dcb6c9a13375d5db73211f250a52c5342d36307d/homeassistant/components/switcher_kis/cover.py#L89 it takes data.position but in this use case it should be something like data.position[Index] You are right for the code you showed.
This for example: https://github.com/home-assistant/core/blob/dcb6c9a13375d5db73211f250a52c5342d36307d/homeassistant/components/switcher_kis/cover.py#L89 it takes data.position but in this use case it should be something like data.position[Index] You are right for the code you showed.
I still don't understand what is the problem, you can subclass SwitcherCoverEntity
in HA and add an index or you can check if the data.position
is a list or not to know if you need to use an index.
This for example: https://github.com/home-assistant/core/blob/dcb6c9a13375d5db73211f250a52c5342d36307d/homeassistant/components/switcher_kis/cover.py#L89 it takes data.position but in this use case it should be something like data.position[Index] You are right for the code you showed.
I still don't understand what is the problem, you can subclass
SwitcherCoverEntity
in HA and add an index or you can check if thedata.position
is a list or not to know if you need to use an index.
Yeah I guess you are right.. it's not a real problem but a matter of adding conditions to support both single value an array of values It's not that nice but it can work
Anyway we go with this PR as it is and we can discuss this later in the PR for HA. You can review this PR when you are ready.
@TomerFi Please approve, merge and make a release. Thanks :)
@YogevBokobza @thecode Sorry for the delay. If we set the release bump to auto, this will be a major release. Are we okay with that, or would you prefer me to manually set this to minor?
I am ok with both options
@YogevBokobza @thecode Sorry for the delay. If we set the release bump to auto, this will be a major release. Are we okay with that, or would you prefer me to manually set this to minor?
Same. I am ok with both options.
My bad - auto will be minor.
I apologize for being a bit busy lately; thank you both for the hard work!
When will this be incorporated in the Home Assistant integration?
When will this be incorporated in the Home Assistant integration?
When this pr will be merged it will be part of HA. Right now we are at block. Sadly, I am not sure if this will be any time soon..
Thanks for the update, and for all your work on this!
Description
Related issue (if any): fixes #585
Checklist
Additional information