Remove unnecessary unit validation #352
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed change
This PR removes the broken unit validation for v2 quirks and actually allows valid units/strings.
Additional information
Fixes #327 and unblocks a lot of v2 quirks.
Supersedes the alternative solutions:
Background on why this change is fine
The issue linked above (#327) contains some more background info. To summarize (and add more):
The ZHA quirks v2 implementation previously didn't allow individually defined units like
PERCENTAGE
orLIGHT_LUX
, as these are not aStrEnum
, but just astr
. Now, we do allow all strings.The ESPHome integration in Home Assistant also doesn't validate units of measurements, but does validate device classes and state classes, which we also do and where the validation actually matters. ESPHome code for reference:
homeassistant/components/esphome/sensor.py#L78-L82
Unlike ESPHome (where a user can use any arbitrary unit of measurement), we review the units used when quirks are merged into the quirks repo, so we already make sure the constants defined in zigpy/ZHA are used (if possible).
Still, it would even be possible to define a custom unit (str), as some official integrations in HA also do, but we haven't needed that for any v2 quirks yet.
This is very similar to how both the HA Core and ZHA repos treat (reviewing) entities/units.
And IIRC, the concerns with the initial quirks v2 implementation were mostly regarding the validation of device classes and state classes, not the unit of measurement.
Future
In the future, we could also consider dropping the units from ZHA and just using the "quirks v2" units from zigpy (and maybe rename them in zigpy, ideally with backwards compatibility to keep custom quirks working..?).
The units are identical to HA and it feels a bit redundant to have them in both ZHA and zigpy. An alternative that was also discussed was a "HA API" package that we can share between ZHA/zigpy/quirks, but we likely want to avoid having another package.