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

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

Conversation

Caius-Bonus
Copy link

This is the counterpart of the original in homeassistant/core: home-assistant/core#109309

@Caius-Bonus Caius-Bonus changed the title Translate enums Allow translations for enum values in EnumSensor and ZHAEnumSelectEntity entities Jul 15, 2024
@Caius-Bonus Caius-Bonus marked this pull request as draft July 15, 2024 15:39
Copy link

codecov bot commented Jul 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.64%. Comparing base (3a1e0d6) to head (1f919ff).
Report is 123 commits behind head on dev.

Additional details and impacted files
@@           Coverage Diff           @@
##              dev      #86   +/-   ##
=======================================
  Coverage   95.64%   95.64%           
=======================================
  Files          61       61           
  Lines        9232     9237    +5     
=======================================
+ Hits         8830     8835    +5     
  Misses        402      402           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

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

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

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

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.

@TheJulianJES TheJulianJES added enhancement New feature or request priority: medium This should be addressed or looked at soon HA changes Related to changes in Home Assistant Core labels Nov 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request HA changes Related to changes in Home Assistant Core priority: medium This should be addressed or looked at soon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants