refactor(entity): drop explicit entity_id override in complex naming - #448
Conversation
| # entity_id only when first creating an entity, so this affects NEW | ||
| # entities only; existing installs are unchanged. getattr keeps it | ||
| # safe if the bundled library predates display_id. | ||
| display_id = getattr(metric, "display_id", metric.unique_id) |
There was a problem hiding this comment.
Why do you need the dynamic getattr? I assume that with the new library you will always have it available.
| # safe if the bundled library predates display_id. | ||
| display_id = getattr(metric, "display_id", metric.unique_id) | ||
| self._attr_unique_id = f"{entity_platform}.{ENTITY_PREFIX}_{installation_id}_{metric.unique_id}" | ||
| self.entity_id = f"{entity_platform}.{ENTITY_PREFIX}_{installation_id}_{display_id}" |
There was a problem hiding this comment.
Do we even need this line? I suspect it is old leftover. We dont do something simialr in the simple naming case so why we need it here? If i'm right than maybe the real fix is just to remove this line and than we dont need the change we made in the library.
|
Thanks for the review!
This PR only changes the suggested id within the complex scheme: from the doubled-prefix If you'd rather drop the deterministic complex scheme altogether and let HA derive entity_ids from names in both modes, then you're right — this line (and |
|
The history was that I started with a single naming which is aligned with what today called complex naming. It is also important to note I was quite a newbie in the HA integration world, so I probably made every mistake possible. Like even setting those entity ids to begin with. I got feedback from users that having the installation id in the entity id is cumbersome for automations / scripting / dashboards / etc. so I wanted to remove it but didnt know how to do it without breaking current users and without breaking users who do have multiple GX devices. In hindsight I was supposed to just set the unique id, let HA handle the entity id and everything would just work. I was just too uneducated to understand that. |
Complex naming was setting self.entity_id to force an installation-id- prefixed entity_id, which tomer-w noted was a historical mistake from before proper HA entity naming patterns were established. Since DEFAULT_SIMPLE_NAMING is True, new installations already get clean HA-derived entity_ids via simple naming. Complex naming only exists for backward compatibility; existing entities keep their already- registered entity_ids regardless. Removing the explicit self.entity_id override lets HA derive entity_ids from the entity name — the same approach simple naming uses. The unique_id in complex naming is unchanged, so entity identity is fully preserved. No display_id needed.
6d2eb06 to
c6a8b47
Compare
pos-ei-don
changed the title
|
You were right — the Simplified this PR to just remove that line: HA now derives the entity_id from the entity name, same as simple naming. No library changes, no new API needed.
The diff is now 5 lines of deletion only. |
|
@pos-ei-don , happy we got this resolved. Can you revert back the changes you made in the library if we dont need them? I dont want to have unused leftovers. |
|
Done — opened tomer-w/victron_mqtt#110 to revert the |
2 participants
You noted in the review that setting
self.entity_idin complex naming was probably a mistake — the pattern from before proper HA entity naming was established. This PR just removes it.What changed:
self.entity_idin the complex-naming branch.Why this is safe:
unique_idis unchanged, so existing entities keep their already-registered entity_id (HA stores it in the entity registry; theunique_idis just the lookup key).DEFAULT_SIMPLE_NAMING = True, so new installations already use simple naming and never hit this path.No library changes needed —
display_id/victron_mqttlibrary not touched.