-
Notifications
You must be signed in to change notification settings - Fork 53
Add postfix number to ZHA entities #525
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
base: dev
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #525 +/- ##
=======================================
Coverage 96.91% 96.91%
=======================================
Files 63 63
Lines 10498 10515 +17
=======================================
+ Hits 10174 10191 +17
Misses 324 324 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, this looks pretty good!
I'll take a closer final look in the next few days, maybe adjust one of the comments, but I think this and the HA implementation are good otherwise.
One note for possible future PRs: it's a bit nicer to commit the regenerated diagnostics separately, since the GitHub diff viewer gets a bit laggy with all the changed files. We squash all PRs when merging them anyway, so it'll still be one commit in the repo.
The Matter integration in Home Assistant has a similar feature. However, it uses the endpoint ID for the postfix. For reference, see: https://github.com/home-assistant/core/blob/180f898bfa8b4bc3a1a5a97f64866c64fe1f551e/homeassistant/components/matter/entity.py#L111 I would not do this in the Zigbee integration, as quite a few Zigbee devices start with endpoint 11 or some other number. This would be confusing if there's "Voltage (11)" and "Voltage (12)", for example. Minor side note: For us, I think the approach in this PR is already the best. |
Thanks for linking this! I'm not sure that approach would work for us here, since we can potentially have almost every entity twice, needing a postfix. Or just once, not needing one. In that case, we'd need two translation key/value pairs per current one, from what I can see, as we'd have a trailing space otherwise. We could make use of it for some quirks v2 defined entities though, where e.g. left/right would be used instead. For these, the entities are defined/fixed per device, so we always know exactly which ones exist. |
Thank you for taking a look and I'm glad that it seems good!
Good point, thanks for the note. I'm used to working with atomic commits that do not break tests but I don't need to do that here if we squash anyways.
I made the same observation and agree.
I also considered implementing it this way, but decided against it for the same reasons @TheJulianJES outlined. |
I don't know if you have thought of this:
With the automatic suffix, you end up with misalignment of the suffix ( Is it really that bad to use the cluster ID? |
This is a PR created based on the discussion in home-assistant/core#151434
Proposed change
This PR implements a feature from the ZHA backlog. It adds an optional
postfix
attribute to ZHA entities. It is used to differentiate between entities that have the same type and name on the same device. The attribute is needed for proper numbered naming in the UI.Additional information
zha/application/platforms/__init__.py
zha/zigbee/device.py
postfix
attribute Add counter to ZHA entity names when a device has multiple of the same type home-assistant/core#151434postfix
attribute toBaseEntityInfo
.Checklist