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

Remove unnecessary unit validation #352

Merged
merged 2 commits into from
Jan 20, 2025

Conversation

TheJulianJES
Copy link
Contributor

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 or LIGHT_LUX, as these are not a StrEnum, but just a str. 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.

@TheJulianJES TheJulianJES added the bugfix This PR fixes a bug label Jan 16, 2025
Copy link

codecov bot commented Jan 16, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.50%. Comparing base (a833f43) to head (4627a84).
Report is 1 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #352      +/-   ##
==========================================
- Coverage   96.51%   96.50%   -0.01%     
==========================================
  Files          61       61              
  Lines        9457     9454       -3     
==========================================
- Hits         9127     9124       -3     
  Misses        330      330              

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

Copy link
Contributor

@prairiesnpr prairiesnpr left a comment

Choose a reason for hiding this comment

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

Looks good, I much prefer this approach.

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

Successfully merging this pull request may close these issues.

validate_unit doesn't allow valid values
2 participants