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.

Sign up for GitHub

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

Allow translations for enum values in EnumSensor and ZHAEnumSelectEntity entities #86

Draft
wants to merge 8 commits into
base: dev
Choose a base branch
from
Draft

Allow translations for enum values in EnumSensor and ZHAEnumSelectEntity entities #86

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2da732f
change translation keys for 2 entities
Jul 15, 2024
cffcda2
change state format to snake_case, instead of 'titles with spaces sta…
Jul 15, 2024
f348c06
fix tests
Jul 15, 2024
0d1d5bd
ruff
Jul 15, 2024
3050415
fix other tests
Jul 15, 2024
f2bc83d
fix tests after temporary fix is removed
Jul 15, 2024
02f63c4
fix test_discovery
Jul 15, 2024
1f919ff
test workaround
Jul 15, 2024
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
4 changes: 2 additions & 2 deletions tests/test_discover.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -647,7 +647,7 @@ class FakeXiaomiAqaraDriverE1(XiaomiAqaraDriverE1):
)
assert (
power_source_entity.state["state"]
== BasicCluster.PowerSource.Mains_single_phase.name
== BasicCluster.PowerSource.Mains_single_phase.name.lower()
)

hook_state_entity = get_entity(
Expand All @@ -656,7 +656,7 @@ class FakeXiaomiAqaraDriverE1(XiaomiAqaraDriverE1):
exact_entity_type=sensor.EnumSensor,
qualifier_func=lambda e: e._enum == AqaraE1HookState,
)
assert hook_state_entity.state["state"] == AqaraE1HookState.Unlocked.name
assert hook_state_entity.state["state"] == AqaraE1HookState.Unlocked.name.lower()

error_detected_entity = get_entity(
zha_device,
Expand Down
36 changes: 24 additions & 12 deletions tests/test_select.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,20 +63,24 @@ async def test_select(
entity = get_entity(zha_device, platform=Platform.SELECT, qualifier=select_name)
assert entity.state["state"] is None # unknown in HA
assert entity.info_object.options == [
"Stop",
"Burglar",
"Fire",
"Emergency",
"Police Panic",
"Fire Panic",
"Emergency Panic",
"stop",
"burglar",
"fire",
"emergency",
"police_panic",
"fire_panic",
"emergency_panic",
]
assert entity._enum == security.IasWd.Warning.WarningMode

# change value from client
await entity.async_select_option(security.IasWd.Warning.WarningMode.Burglar.name)
await entity.async_select_option(
security.IasWd.Warning.WarningMode.Burglar.name.lower()
)
await zha_gateway.async_block_till_done()
assert entity.state["state"] == security.IasWd.Warning.WarningMode.Burglar.name
assert (
entity.state["state"] == security.IasWd.Warning.WarningMode.Burglar.name.lower()
)


class MotionSensitivityQuirk(CustomDevice):
Expand Down Expand Up @@ -251,11 +255,19 @@ async def test_non_zcl_select_state_restoration(
assert entity.state["state"] is None

entity.restore_external_state_attributes(
state=security.IasWd.Warning.WarningMode.Burglar.name
state=security.IasWd.Warning.WarningMode.Burglar.name.lower()
)
assert (
entity.state["state"] == security.IasWd.Warning.WarningMode.Burglar.name.lower()
)

entity.restore_external_state_attributes(
state=security.IasWd.Warning.WarningMode.Fire.name.lower()
)
assert entity.state["state"] == security.IasWd.Warning.WarningMode.Burglar.name
assert entity.state["state"] == security.IasWd.Warning.WarningMode.Fire.name.lower()

# test workaround for existing installations updating
entity.restore_external_state_attributes(
state=security.IasWd.Warning.WarningMode.Fire.name
)
assert entity.state["state"] == security.IasWd.Warning.WarningMode.Fire.name
assert entity.state["state"] == security.IasWd.Warning.WarningMode.Fire.name.lower()
2 changes: 1 addition & 1 deletion tests/test_sensor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,7 @@ async def async_test_setpoint_change_source(
cluster,
{hvac.Thermostat.AttributeDefs.setpoint_change_source.id: 0x01},
)
assert entity.state["state"] == "Schedule"
assert entity.state["state"] == "schedule"


async def async_test_pi_heating_demand(
Expand Down
19 changes: 13 additions & 6 deletions zha/application/platforms/select.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ class EnumSelectEntity(PlatformEntity):
_attr_entity_category = EntityCategory.CONFIG
_attribute_name: str
_enum: type[Enum]
_translation_keys: dict[str, str]

def __init__(
self,
Expand All @@ -75,7 +76,10 @@ def __init__(
"""Init this select entity."""
self._cluster_handler: ClusterHandler = cluster_handlers[0]
self._attribute_name = self._enum.__name__
self._attr_options = [entry.name.replace("_", " ") for entry in self._enum]
self._translation_keys = {
entry.name.lower(): entry.name for entry in self._enum
}
self._attr_options = [entry.name.lower() for entry in self._enum]
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this be a breaking change for entities that do not yet implement translation keys?

Copy link
Author

@Caius-Bonus Caius-Bonus Jul 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In homeassistant/core, I have written translation keys for all enums. But unfortunately, regardless of this specific code fragment, the internal state will change from the current form (titles with the first letter being a capital) to snake_case without any capitals. Automations that depend on this will seize to function.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. If it's going to be a breaking change we should get these strings into the exact state we want them (with no compatibility shims) and then prominently mark the breaking change as such.

Copy link
Author

Choose a reason for hiding this comment

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

Currently, the only thing we do is .lower() as most Enums have capitals in them. Do you see that as a compatibility shim? Changing that requires changes in at least: zigpy/zha, zigpy/zigpy and zigpy/zha-device-handlers. There might be more users of zigpy/zigpy that rely on the names in the Enums.

Copy link
Author

@Caius-Bonus Caius-Bonus Jul 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Secondly, if we want to create final names, we probably should change the CamelCase Enums to snake_case (CamelCase looks pretty bad in lowercase), which aren't many, but they do exist in zigpy/zha and zigpy/zha-device-handlers.
Thirdly, maybe this is also a nice opportunity to move the Enums still present in zigpy/zha to zigpy/zha-device-handlers, while changing the naming where appropriate.

Copy link
Contributor

Choose a reason for hiding this comment

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

SomeValue would become somevalue instead of some_value. I think in zigpy we use a mix of UPPERCASE_WITH_UNDERSCORES, CamelCase, and Whatever_case_this_is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I see. They're all still valid at least 😄
So, for HA state/translation keys, we probably always want some_value.

Which style do we want to use for the actual enums in general? Also some_value?
Or do we want to leave them as-is for now and just convert all types you mentioned to some_value in ZHA? Should be possible at least.. (We could change the actual enums to some_value (or similar) later, without breaking HA state again then.)

Copy link
Author

@Caius-Bonus Caius-Bonus Oct 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

When leaving the actual enums as-is, from "SomeValue" we would get "somevalue" using .lower(). So when changing the actual enums later on to "some_value", we would still have a breaking change. Although we could also do something like: re.sub(r"_+", r"_", re.sub(r"([A-Z])", r"_\1", "SomeValue").strip("_").lower()).

Edit: fix multiple underscores for occurrences of "Some_Value".

Copy link
Contributor

Choose a reason for hiding this comment

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

Here's all of the enum members defined in quirks and zigpy: https://gist.github.com/puddly/2190cc8450bbcb6200afd6b87e516252.

I think the function in there should work, I didn't see any mistakes for this mapping:

{
    "FooBar": "foo_bar",
    "FOOBAR": "foobar",
    "FOO_BAR": "foo_bar",
    "foo_bar": "foo_bar",
    "FOOBar": "foo_bar"
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, let's use this.

super().__init__(unique_id, cluster_handlers, endpoint, device, **kwargs)

@functools.cached_property
Expand All @@ -100,12 +104,12 @@ def current_option(self) -> str | None:
option = self._cluster_handler.data_cache.get(self._attribute_name)
if option is None:
return None
return option.name.replace("_", " ")
return option.name.lower()

async def async_select_option(self, option: str) -> None:
"""Change the selected option."""
self._cluster_handler.data_cache[self._attribute_name] = self._enum[
option.replace(" ", "_")
self._translation_keys[option]
]
self.maybe_emit_state_changed_event()

Expand All @@ -115,7 +119,10 @@ def restore_external_state_attributes(
state: str,
) -> None:
"""Restore extra state attributes that are stored outside of the ZCL cache."""
value = state.replace(" ", "_")
try:
value = self._translation_keys[state]
except KeyError: # workaround for existing installations updating
value = state.replace(" ", "_")
self._cluster_handler.data_cache[self._attribute_name] = self._enum[value]


Expand Down Expand Up @@ -485,7 +492,7 @@ class AqaraT2RelaySwitchType(ZCLEnumSelectEntity):
_unique_id_suffix = "switch_type"
_attribute_name = "switch_type"
_enum = T2RelayOppleCluster.SwitchType
_attr_translation_key: str = "switch_type"
_attr_translation_key: str = "relay_switch_type"


@CONFIG_DIAGNOSTIC_MATCH(
Expand Down Expand Up @@ -549,7 +556,7 @@ class InovelliSwitchTypeEntity(ZCLEnumSelectEntity):
_unique_id_suffix = "switch_type"
_attribute_name = "switch_type"
_enum = InovelliSwitchType
_attr_translation_key: str = "switch_type"
_attr_translation_key: str = "fan_switch_type"


class InovelliFanSwitchType(types.enum1):
Expand Down
4 changes: 2 additions & 2 deletions zha/application/platforms/sensor/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,7 +458,7 @@ def __init__(
) -> None:
"""Init this sensor."""
super().__init__(unique_id, cluster_handlers, endpoint, device, **kwargs)
self._attr_options = [e.name for e in self._enum]
self._attr_options = [e.name.lower() for e in self._enum]

# XXX: This class is not meant to be initialized directly, as `unique_id`
# depends on the value of `_attribute_name`
Expand All @@ -473,7 +473,7 @@ def _init_from_quirks_metadata(self, entity_metadata: ZCLEnumMetadata) -> None:
def formatter(self, value: int) -> str | None:
"""Use name of enum."""
assert self._enum is not None
return self._enum(value).name
return self._enum(value).name.lower()


@MULTI_MATCH(
Expand Down
Loading