Device registry store config entry by Kane610 · Pull Request #16152 · home-assistant/core

Kane610 · 2018-08-23T18:56:17Z

Description:

Related issue (if applicable): fixes #16183

Checklist:

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

If the code does not interact with devices:

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

balloob · 2018-08-23T20:17:31Z

homeassistant/helpers/device_registry.py

Trigger save if it didn't contain the config entry.

balloob · 2018-08-23T20:19:00Z

homeassistant/helpers/device_registry.py

Shouldn't identifiers and connection also be sets of tuples?

set([ ('mac', 'aa:bb'), ('deconz', 123) ])

Yes, lets do it properly now :)

MartinHjelmare · 2018-08-24T05:34:35Z

Renaming and retyping has made device parameters a lot clearer. 👍

balloob · 2018-08-24T08:32:17Z

homeassistant/helpers/device_registry.py

I think default should be a function, not a set?

Im not following, this works as expected for an attribute that might not be set on creation.

What should the function solve @balloob ?

The set will be shared between all instances? See the 2nd code snippet in the docs. You need to do factory=set instead.

Maybe it's working in this case because the empty set is passed into converter, which will construct a new set.

I will add a factory

balloob · 2018-08-24T08:33:30Z

homeassistant/helpers/device_registry.py

We should only save if the device has changed.

balloob · 2018-08-24T08:35:37Z

tests/helpers/test_device_registry.py

for the sake of giving good examples, we should not use bridgeid here. All identifiers need to be scoped and not be generic names. If Hue and Deconz use same bridge ids, it shouldn't match devices.

That is improved by the deconz device registry PR. I prefer to not duplicate implementation between two different PRs.

balloob · 2018-08-24T14:04:33Z

homeassistant/helpers/device_registry.py

Let's make config entry mandatory (not default to None). Otherwise we don't know when to clean up devices etc. Now it's easy as on removal of a config entry we can delete things.

Cleaning up devices on removal of a config entry is for another PR btw ;)

What about devices not having a config entry?

I've been starting to look at removing devices in a separate PR 👍

houndci-bot · 2018-08-24T15:16:57Z

tests/helpers/test_device_registry.py

dictionary key 'config_entries' repeated with different values

houndci-bot · 2018-08-24T15:16:57Z

tests/helpers/test_device_registry.py

dictionary key 'config_entries' repeated with different values

…exists

balloob · 2018-08-24T21:52:54Z

homeassistant/helpers/entity_platform.py

-                    name=device.get('name'),
-                    sw_version=device.get('sw_version'))
+                    config_entry=config_entry_id,
+                    connections=device_info['connections'],


This will blow up if the key connections, identifiers or manufacturer does not exist. We should use device_info.get('connections', []).

We should probably be defensive for all values and use get

Yes, we already have an issue report on this it seems :)

balloob · 2018-08-24T21:53:46Z

homeassistant/helpers/device_registry.py

+                self.async_schedule_save()
            return device

        device = DeviceEntry(


We should not create a new device if it has no connections or identifiers, as it can never be retrieved again

Should identifiers be mandatory or should it check and fail for any of them?

I guess you're right that at least one is mandatory, I am fine with ignoring it when it doesn't match and just print a warning

balloob

Ok to merge when added that we don't save when we have no connection or identifier

Kane610 · 2018-08-25T06:06:14Z

@balloob isn't the top if statement doing just that? If someone else triggers save later it will still be stored

* Allow device registry to optionally store config entries * Connections and identifiers are now sets with tupels * Make config entries mandatory * Fix duplicate keys in test * Rename device to device_info * Entity platform should only create device entries if config_entry_id exists * Fix Soundtouch tests * Revert soundtouch to use self.device * Fix baloobs comments * Correct type in test

Kane610 requested a review from a team as a code owner August 23, 2018 18:56

homeassistant added core cla-signed labels Aug 23, 2018

ghost assigned Kane610 Aug 23, 2018

ghost added the in progress label Aug 23, 2018

balloob reviewed Aug 23, 2018

View reviewed changes

homeassistant added the cla-signed label Aug 23, 2018

balloob reviewed Aug 24, 2018

View reviewed changes

balloob added this to the 0.77 milestone Aug 24, 2018

homeassistant added the cla-signed label Aug 24, 2018

houndci-bot reviewed Aug 24, 2018

View reviewed changes

balloob approved these changes Aug 24, 2018

View reviewed changes

Kane610 changed the title ~~Allow device registry to optionally store config entries~~ Device registry require config entries Aug 24, 2018

Kane610 changed the title ~~Device registry require config entries~~ Device registry store config entry Aug 24, 2018

homeassistant added the cla-signed label Aug 24, 2018

Kane610 added 7 commits August 24, 2018 19:43

Allow device registry to optionally store config entries

a065271

Connections and identifiers are now sets with tupels

936c14c

Make config entries mandatory

f141039

Fix duplicate keys in test

b8adc43

Rename device to device_info

4ab222b

Entity platform should only create device entries if config_entry_id …

11475f2

…exists

Fix Soundtouch tests

2f34d27

Kane610 force-pushed the device-registry-store-config-entry branch from c9882fa to 2f34d27 Compare August 24, 2018 18:22

homeassistant added the cla-signed label Aug 24, 2018

Revert soundtouch to use self.device

e2d88fb

balloob reviewed Aug 24, 2018

View reviewed changes

Kane610 added 2 commits August 25, 2018 02:08

Fix baloobs comments

a269164

Correct type in test

728a72c

Kane610 mentioned this pull request Aug 25, 2018

Entity platform error in 0.77.0b0 #16183

Closed

balloob approved these changes Aug 25, 2018

View reviewed changes

balloob merged commit 97173f4 into home-assistant:dev Aug 25, 2018

ghost removed the in progress label Aug 25, 2018

balloob added the cherry-picked label Aug 25, 2018

Kane610 deleted the device-registry-store-config-entry branch August 29, 2018 08:30

balloob mentioned this pull request Aug 29, 2018

0.77 #16256

Merged

home-assistant locked and limited conversation to collaborators Dec 10, 2018

Uh oh!

Conversation

Kane610 commented Aug 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Checklist:

Uh oh!

balloob Aug 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare commented Aug 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kane610 Aug 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kane610 Aug 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balloob left a comment

Choose a reason for hiding this comment

Uh oh!

Kane610 commented Aug 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Kane610 commented Aug 23, 2018 •

edited

Loading

balloob Aug 23, 2018 •

edited

Loading

Kane610 Aug 24, 2018 •

edited

Loading

Kane610 Aug 24, 2018 •

edited

Loading