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

Refactor switch for vesync #134409

Merged
merged 33 commits into from
Feb 5, 2025
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
0d67c7b
Refactor switch sensor so we can use it for more.
cdnninja Dec 30, 2024
033cac8
Merge branch 'dev' into sensorRefactor
cdnninja Jan 1, 2025
3a663de
Adjust order so all platforms match between PRs
cdnninja Jan 2, 2025
6604d26
Correct type
cdnninja Jan 2, 2025
f4352fd
Attempt to fix mypy error
cdnninja Jan 2, 2025
f143594
Correct mypy erros.
cdnninja Jan 2, 2025
1337372
Correct Errors
cdnninja Jan 2, 2025
7b0510d
Add callbacks
cdnninja Jan 2, 2025
8db10fb
Merge branch 'dev' into sensorRefactor
cdnninja Jan 3, 2025
0ce2086
Add coordinator as per other PR
cdnninja Jan 3, 2025
0fc9e81
Merge branch 'dev' into sensorRefactor
cdnninja Jan 5, 2025
4a2fa3f
Merge issue
cdnninja Jan 5, 2025
0288768
ruff
cdnninja Jan 6, 2025
c12b55d
Fedback
cdnninja Jan 6, 2025
9043ba7
Merge branch 'dev' into sensorRefactor
cdnninja Jan 15, 2025
ff5f13c
Ruff correction
cdnninja Jan 15, 2025
8f13594
Add exists FN
cdnninja Jan 15, 2025
5b08b11
Corrections to pass tests
cdnninja Jan 15, 2025
e32f14f
Remove dimmable switch from scope to pass tests.
cdnninja Jan 15, 2025
139d4e1
Remove unused code to increase test coverage
cdnninja Jan 15, 2025
10e9925
switch to async updates and refresh
cdnninja Jan 18, 2025
8b38c47
Revert last change
cdnninja Jan 19, 2025
85fdfdd
Update homeassistant/components/vesync/common.py
cdnninja Jan 19, 2025
70e2cdf
Update homeassistant/components/vesync/switch.py
cdnninja Jan 19, 2025
c6d2088
Feedback Items
cdnninja Jan 19, 2025
27c255e
Improved comments.
cdnninja Jan 19, 2025
1013880
feedback elif
cdnninja Jan 25, 2025
51d1ead
Feedback
cdnninja Jan 25, 2025
d963ec7
Add unique ID
cdnninja Jan 29, 2025
b39778e
Merge branch 'dev' into sensorRefactor
cdnninja Feb 3, 2025
bde6507
If for on / off
cdnninja Feb 3, 2025
765b86b
remove mistake
cdnninja Feb 4, 2025
918ff0e
Ruff
cdnninja Feb 4, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions homeassistant/components/vesync/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,28 @@
_LOGGER = logging.getLogger(__name__)


def rgetattr(obj, attr):
"""Return a string in the form word.1.2.3 and return the item as 3. Note that this last value could be in a dict as well."""
_this_func = rgetattr
sp = attr.split(".", 1)
if len(sp) == 1:
left, right = sp[0], ""
else:
left, right = sp

Check warning on line 22 in homeassistant/components/vesync/common.py

View check run for this annotation

Codecov / codecov/patch

homeassistant/components/vesync/common.py#L22

Added line #L22 was not covered by tests

if isinstance(obj, dict):
obj = obj.get(left)

Check warning on line 25 in homeassistant/components/vesync/common.py

View check run for this annotation

Codecov / codecov/patch

homeassistant/components/vesync/common.py#L25

Added line #L25 was not covered by tests
elif hasattr(obj, left):
obj = getattr(obj, left)
else:
return None

Check warning on line 29 in homeassistant/components/vesync/common.py

View check run for this annotation

Codecov / codecov/patch

homeassistant/components/vesync/common.py#L29

Added line #L29 was not covered by tests

if right:
obj = _this_func(obj, right)

Check warning on line 32 in homeassistant/components/vesync/common.py

View check run for this annotation

Codecov / codecov/patch

homeassistant/components/vesync/common.py#L32

Added line #L32 was not covered by tests

return obj


async def async_process_devices(
hass: HomeAssistant, manager: VeSync
) -> dict[str, list[VeSyncBaseDevice]]:
Expand Down
5 changes: 5 additions & 0 deletions homeassistant/components/vesync/strings.json
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,11 @@
"name": "Current voltage"
}
},
"switch": {
"on": {
"name": "On"
}
},
cdnninja marked this conversation as resolved.
Show resolved Hide resolved
"fan": {
"vesync": {
"state_attributes": {
Expand Down
114 changes: 64 additions & 50 deletions homeassistant/components/vesync/switch.py
Original file line number Diff line number Diff line change
@@ -1,29 +1,54 @@
"""Support for VeSync switches."""

from collections.abc import Callable
from dataclasses import dataclass
import logging
from typing import Any
from typing import Any, Final

from pyvesync.vesyncbasedevice import VeSyncBaseDevice

from homeassistant.components.switch import SwitchEntity
from pyvesync.vesyncoutlet import VeSyncOutlet
from pyvesync.vesyncswitch import VeSyncSwitch

from homeassistant.components.switch import (
SwitchDeviceClass,
SwitchEntity,
SwitchEntityDescription,
)
from homeassistant.config_entries import ConfigEntry
from homeassistant.core import HomeAssistant, callback
from homeassistant.helpers.dispatcher import async_dispatcher_connect
from homeassistant.helpers.entity_platform import AddEntitiesCallback

from .const import DEV_TYPE_TO_HA, DOMAIN, VS_COORDINATOR, VS_DISCOVERY, VS_SWITCHES
from .common import rgetattr
from .const import DOMAIN, VS_COORDINATOR, VS_DISCOVERY, VS_SWITCHES
from .coordinator import VeSyncDataCoordinator
from .entity import VeSyncBaseEntity

_LOGGER = logging.getLogger(__name__)


@dataclass(frozen=True, kw_only=True)
class VeSyncSwitchEntityDescription(SwitchEntityDescription):
"""A class that describes custom switch entities."""

is_on: Callable[[VeSyncBaseDevice], bool]


SENSOR_DESCRIPTIONS: Final[tuple[VeSyncSwitchEntityDescription, ...]] = (
VeSyncSwitchEntityDescription(
key="device_status",
translation_key="on",
is_on=lambda device: device.device_status == "on",
),
cdnninja marked this conversation as resolved.
Show resolved Hide resolved
)


async def async_setup_entry(
hass: HomeAssistant,
config_entry: ConfigEntry,
async_add_entities: AddEntitiesCallback,
) -> None:
"""Set up switches."""
"""Set up switch platform."""

coordinator = hass.data[DOMAIN][VS_COORDINATOR]

Expand All @@ -46,57 +71,46 @@
coordinator: VeSyncDataCoordinator,
):
"""Check if device is online and add entity."""
entities: list[VeSyncBaseSwitch] = []
for dev in devices:
if DEV_TYPE_TO_HA.get(dev.device_type) == "outlet":
entities.append(VeSyncSwitchHA(dev, coordinator))
elif DEV_TYPE_TO_HA.get(dev.device_type) == "switch":
entities.append(VeSyncLightSwitch(dev, coordinator))
else:
_LOGGER.warning(
"%s - Unknown device type - %s", dev.device_name, dev.device_type
)
continue

async_add_entities(entities, update_before_add=True)

async_add_entities(
(
VeSyncSwitchEntity(dev, description, coordinator)
for dev in devices
for description in SENSOR_DESCRIPTIONS
if rgetattr(dev, description.key) is not None
),
update_before_add=True,
cdnninja marked this conversation as resolved.
Show resolved Hide resolved
)

class VeSyncBaseSwitch(VeSyncBaseEntity, SwitchEntity):
"""Base class for VeSync switch Device Representations."""

_attr_name = None
class VeSyncSwitchEntity(SwitchEntity, VeSyncBaseEntity):
"""VeSync sensor class."""
cdnninja marked this conversation as resolved.
Show resolved Hide resolved

def turn_on(self, **kwargs: Any) -> None:
"""Turn the device on."""
self.device.turn_on()
def __init__(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to define _attr_unique_id otherwise adding another switch entity fails.

Something like this will work - self._attr_unique_id = f"{super().unique_id}-{description.key}"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the two devices exposed today if we change this it will be a breaking change. However I could add a filter to only do the above unique ID on ones other than the outlet and switch on we offer today. It would set us up for future.

Thoughts on that approach?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose that will work. @joostlek might have reviewed code which ran into similar shortcoming and if the _attr_unique_id was defined as a breaking change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that is a good catch. If that is the case we should migrate first. I don't feel like merging this as it will create entities with not unique unique ids. So we should have a preliminary PR where we migrate all the unique ids of the switches to something else and then we can set up new ones

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, today this code doesn't conflict or impact unique ID, however since trying to setup for growth makes sense we fix this now. I will update this code to have the unique ID as mentioned above and chat with iprak around getting a PR for that migration that will merge first.

self,
device: VeSyncBaseDevice,
description: VeSyncSwitchEntityDescription,
coordinator: VeSyncDataCoordinator,
) -> None:
"""Initialize the sensor."""
super().__init__(device, coordinator)
self.entity_description: VeSyncSwitchEntityDescription = description
cdnninja marked this conversation as resolved.
Show resolved Hide resolved
if isinstance(self.device, VeSyncOutlet):
self._attr_device_class = SwitchDeviceClass.OUTLET
if isinstance(self.device, VeSyncSwitch):
self._attr_device_class = SwitchDeviceClass.SWITCH
cdnninja marked this conversation as resolved.
Show resolved Hide resolved
self._attr_name = None

@property
def is_on(self) -> bool:
"""Return True if device is on."""
return self.device.device_status == "on"
def is_on(self) -> bool | None:
"""Return the entity value to represent the entity state."""
if self.entity_description.is_on is not None:
return self.entity_description.is_on(self.device)
return None

Check warning on line 108 in homeassistant/components/vesync/switch.py

View check run for this annotation

Codecov / codecov/patch

homeassistant/components/vesync/switch.py#L108

Added line #L108 was not covered by tests
cdnninja marked this conversation as resolved.
Show resolved Hide resolved

def turn_off(self, **kwargs: Any) -> None:
"""Turn the device off."""
"""Turn the entity off."""
self.device.turn_off()


class VeSyncSwitchHA(VeSyncBaseSwitch, SwitchEntity):
"""Representation of a VeSync switch."""

def __init__(
self, plug: VeSyncBaseDevice, coordinator: VeSyncDataCoordinator
) -> None:
"""Initialize the VeSync switch device."""
super().__init__(plug, coordinator)
self.smartplug = plug


class VeSyncLightSwitch(VeSyncBaseSwitch, SwitchEntity):
"""Handle representation of VeSync Light Switch."""

def __init__(
self, switch: VeSyncBaseDevice, coordinator: VeSyncDataCoordinator
) -> None:
"""Initialize Light Switch device class."""
super().__init__(switch, coordinator)
self.switch = switch
def turn_on(self, **kwargs: Any) -> None:
"""Turn the entity on."""
self.device.turn_on()

Check warning on line 116 in homeassistant/components/vesync/switch.py

View check run for this annotation

Codecov / codecov/patch

homeassistant/components/vesync/switch.py#L116

Added line #L116 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't schedule_update_ha_state be only called if turn_on succeeds?

10 changes: 6 additions & 4 deletions tests/components/vesync/snapshots/test_switch.ambr
Original file line number Diff line number Diff line change
Expand Up @@ -300,13 +300,13 @@
'name': None,
'options': dict({
}),
'original_device_class': None,
'original_device_class': <SwitchDeviceClass.OUTLET: 'outlet'>,
'original_icon': None,
'original_name': None,
'platform': 'vesync',
'previous_unique_id': None,
'supported_features': 0,
'translation_key': None,
'translation_key': 'on',
'unique_id': 'outlet',
'unit_of_measurement': None,
}),
Expand All @@ -315,6 +315,7 @@
# name: test_switch_state[Outlet][switch.outlet]
StateSnapshot({
'attributes': ReadOnlyDict({
'device_class': 'outlet',
'friendly_name': 'Outlet',
}),
'context': <ANY>,
Expand Down Expand Up @@ -420,13 +421,13 @@
'name': None,
'options': dict({
}),
'original_device_class': None,
'original_device_class': <SwitchDeviceClass.SWITCH: 'switch'>,
'original_icon': None,
'original_name': None,
'platform': 'vesync',
'previous_unique_id': None,
'supported_features': 0,
'translation_key': None,
'translation_key': 'on',
'unique_id': 'switch',
'unit_of_measurement': None,
}),
Expand All @@ -435,6 +436,7 @@
# name: test_switch_state[Wall Switch][switch.wall_switch]
StateSnapshot({
'attributes': ReadOnlyDict({
'device_class': 'switch',
'friendly_name': 'Wall Switch',
}),
'context': <ANY>,
Expand Down