-
-
Notifications
You must be signed in to change notification settings - Fork 31.2k
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
Enable new registry rename for Insteon #17171
Conversation
Hi @wonderslug, It seems you haven't yet signed a CLA. Please do so here. Once you do that we will be able to review and accept this pull request. Thanks! |
Closing for Re-Run of CI after Error (not failure) |
Reopen for Re-Run of CI after Error (not failure) |
@property | ||
def unique_id(self) -> str: | ||
"""Return a unique ID.""" | ||
return self.name |
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.
This is weird, we should just remove self.name as it's confusing for users and move the definition to unique_id property. Don't we have access to a real name for the name property?
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.
From an Insteon stand point name gets the underlying device id with group for multi referenced nodes (button id or sub device) and is the "real name". It is also unique within an Insteon network and can act as the uinique_id for the registry.
Do the name and unique_id need to be different?
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.
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.
Of note doing this does make a breaking change for entity_ids being different for existing customizations.
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.
We don't need any of those node IDs anymore, once there is unique ID, people can change it. And it will also make sure there is never a collision of IDs, they get reserved.
So name should be the most natural name one can think of. Yes, that will be a breaking change so we should do this in 2 phases: first just introduce the unique ID that is the same as the name which tries to be unique.
Then in a couple of releases, we can drop the name being all weird, because everyone who has upgraded already has their entity ID locked in, making it a non-breaking change.
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.
Still I want to make sure we copy the implementation of name property because I don't trust that in the future someone will jsut change the name, messing with the unique IDs, and now that would suck.
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.
Not a problem. Ill make the unique_id distinct from name and ill put up a separate issue and PR as WIP with the phase 2 up.
I just came here to file an issue about this. I think this is exactly what I was looking for! |
## Description:
This is a simple change to the InsteonEntity what supports the unique_id property in order to allow renames with the new entity registry.
Currently the name returns a unique name thats used for the entity_id. This is just being called now by the unique_id property in order to allow for the registry to support the rename configuration options.
Related issue (if applicable): fixes #17170
Pull request in home-assistant.io with documentation (if applicable): None
Checklist:
tox
. Your PR cannot be merged unless tests passIf user exposed functionality or configuration variables are added/changed: None Needed
If the code communicates with devices, web services, or third-party tools: None Needed
If the code does not interact with devices: