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

Enable new registry rename for Insteon #17171

Merged
merged 2 commits into from Oct 7, 2018
Merged

Enable new registry rename for Insteon #17171

merged 2 commits into from Oct 7, 2018

Conversation

wonderslug
Copy link
Contributor

## 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:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

If 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:

  • Tests have been added to verify that the new code works.

@homeassistant
Copy link
Contributor

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!

@ghost ghost added the in progress label Oct 5, 2018
@wonderslug wonderslug changed the title Enable new registry rename for Insteon (#17170) Enable new registry rename for Insteon Oct 5, 2018
@wonderslug
Copy link
Contributor Author

Closing for Re-Run of CI after Error (not failure)

@wonderslug wonderslug closed this Oct 5, 2018
@ghost ghost removed the in progress label Oct 5, 2018
@wonderslug
Copy link
Contributor Author

Reopen for Re-Run of CI after Error (not failure)

@wonderslug wonderslug reopened this Oct 5, 2018
@ghost ghost added the in progress label Oct 5, 2018
@property
def unique_id(self) -> str:
"""Return a unique ID."""
return self.name
Copy link
Member

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?

Copy link
Contributor Author

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we can get a device description, model, and Insteon address kinda like zwave does for its nodes.

So name would become something like
"Keypad Dimmer 2334-222 291abb_2",
"I/O Linc 2450 440f9c"
"Door Sensor 2843-232 21545c"

This ends up looking like this
Example

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

@wonderslug
Copy link
Contributor Author

Name changes added to #17194 and PR #17195

@dwradcliffe
Copy link
Contributor

I just came here to file an issue about this. I think this is exactly what I was looking for!

@balloob balloob merged commit 592e1dc into home-assistant:dev Oct 7, 2018
@ghost ghost removed the in progress label Oct 7, 2018
@wonderslug wonderslug deleted the insteon_rename branch October 10, 2018 20:23
@balloob balloob mentioned this pull request Oct 26, 2018
@home-assistant home-assistant locked and limited conversation to collaborators Feb 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Device ID Rename not available for Insteon
4 participants