Refactor switch for vesync #134409
Refactor switch for vesync #134409
Conversation
|
Hey there @markperdue, @webdjoe, @TheGardenMonkey, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
|
@iprak FYI. As two side notes you should add yourself as a code owner on this integration, your support has been incredible. Would love to connect more on the HA discord too. |
|
I am thinking this PR should probably split the removal of the VeSyncDevice entity to a different PR. I don't see value in that class. Just complicates things. Thoughts? |
cdnninja
changed the title
|
My one thought is I am not certain the names of the entities are unchanged for the two existing. I also can't test that hardware as I don't own it. |
Sure, it definitely helps to keep PR small. |
Other portion has been split to #134499. |
| VeSyncSwitchEntityDescription( | ||
| key="device_status", | ||
| translation_key="on", | ||
| is_on=lambda device: device.device_status == "on", | ||
| ), |
There was a problem hiding this comment.
Should we just use the power device class? We should either set the name to None (causing the entity to follow the device name, so switch.vesync_device) or if there already is another main feature of the device (say a climate entity or humidifier), we should use the device class translation for the POWER device class
There was a problem hiding this comment.
Any harm in us not filtering if say it is a fan or humidifer? name would be switch.vesync_device and allow a toggle for power. This keeps the code simple as I don't need to filter if one of those.
There was a problem hiding this comment.
Duplicate entity? Humidifier and fan both derive from ToggleEntity and expose the same actions.
There was a problem hiding this comment.
I will test this in conjunction with humidifier PR once that one is merged. Lets plan to merge this after that one.
There was a problem hiding this comment.
Sorry but why do we need this entity?
The device_status in pyvesync seems to reflect the on/off state which fan/humidifier entities will report as well via is_on. They also have turn_on/turn_off to change the status. I did not check but I think the same should be true for light entities.
There was a problem hiding this comment.
They used to sell outlets and switches. As per: https://www.home-assistant.io/integrations/vesync/ so this would show on off for the light switch or outlet. I am thinking I should add a filter ability so it can be removed for select situations. In the future this switch platform can offer other items though, which is the driver for the refactor.
There was a problem hiding this comment.
Switch and outlet would need that but I don't think they should be needed for fan/humidifiers.
My humidifier appears as following with off state.
There was a problem hiding this comment.
I will add an additional check so we can adjust for that. I won't get to this right a way since I think humidifier merge and binary_sensor take priority.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Proposed change
Refactor of switch sensor to support it offering more switches in the future, such as screen on off. This one will wait on the update coordinator merge first then update to reflect those changes. Creating a draft PR for early feedback.
I do not own a vesync switch or outlet so I would love to have someone test this on real hardware.
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: