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

validate_unit doesn't allow valid values #327

Open
TheJulianJES opened this issue Dec 12, 2024 · 3 comments · May be fixed by #352
Open

validate_unit doesn't allow valid values #327

TheJulianJES opened this issue Dec 12, 2024 · 3 comments · May be fixed by #352
Labels
bug Confirmed bug

Comments

@TheJulianJES
Copy link
Contributor

First, validate_unit typing is Enum, although it's typed as str everywhere else, so in quirks v2 metadata, and so on.
In ZHA and HA, _attr_native_unit_of_measurement is also typed as str | None.

It should be changed to accept and return a str IMO.

The mapping is also not ideal, as we have units replicated in both zigpy and ZHA now. We should change this in the future, e.g. by just having it in zigpy (and maybe renaming the quirks/v2 where it is currently to something else). Or a separate lib?

We also need to fix not accepting the units strings that are defined, like PERCENTAGE. We need to allow those.
Actually, I don't think we really need the validation for a string anyways. Quirks v2 entities are still reviewed and obviously need to use the constants defined here before they can be merged.
HA might also already validate the units used?

Link to method (and units file):

zha/zha/units.py

Lines 179 to 181 in c013c5b

def validate_unit(external_unit: Enum) -> Enum:
"""Validate and return a unit of measure."""
return UNITS_OF_MEASURE[type(external_unit).__name__](external_unit.value)

cc @prairiesnpr

@TheJulianJES TheJulianJES added the bug Confirmed bug label Dec 12, 2024
@TheJulianJES
Copy link
Contributor Author

I'll have to double check, but I don't think ESPHome validates the provided unit_of_measurement.
HA accepts any values, even something like steps. Unit translations were also added for units like that.

In the worst case, HA logs a warning if the unit doesn't match the state class. The state class and the unit playing together is the important part. If there's no state class, all units should be valid.
Still, we review v2 quirks before merging them, so state class and units have to match. We don't have this validation for entities added in ZHA (and it's reviewed by the same people as v2 quirks).

So, we really should just drop this validation for units IMO. There's nothing positive we get from it.

@prairiesnpr
Copy link
Contributor

So, we really should just drop this validation for units IMO. There's nothing positive we get from it.

Concur, I don't see a reason to validate the value in ZHA, it just adds complexity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants