Make device info a TypedDict #49670
Make device info a TypedDict #49670
Conversation
|
Hey there @fphammerle, mind taking a look at this pull request as its been labeled with an integration ( |
|
Hey there @home-assistant/z-wave, mind taking a look at this pull request as its been labeled with an integration ( |
| "identifiers": {(DOMAIN, self._hub.bond_id, self._device.device_id)}, | ||
| "suggested_area": self._device.location, | ||
| "via_device": (DOMAIN, self._hub.bond_id), | ||
| "identifiers": { |
There was a problem hiding this comment.
The dev docs aren't really clear that this isn't allowed for identifiers, but it does seem explicit for connections
https://developers.home-assistant.io/docs/device_registry_index/#device-properties
AFAICT, the only requirement for identifiers is they have to be unique and hashable
There was a problem hiding this comment.
I think it would be ok to limit it to a two string tuple. There should be no problem to concatenate the strings into a single string as needed for the second string item.
There was a problem hiding this comment.
We should probably update the docs as well to make it explict.
There was a problem hiding this comment.
Device registry has identifiers hinted as str two-tuples everywhere though. One approach to fixing this would be to relax that somehow, but I don't know if that's seen appropriate or what's the intent.
There was a problem hiding this comment.
I think it would be ok to limit it to a two string tuple. There should be no problem to concatenate the strings into a single string as needed for the second string item.
Agreed, but I suppose just doing that would cause the newly structured identifiers not match their old saved device registry entries for the bond integration, some migration code would be needed.
There was a problem hiding this comment.
We haven't enforced two string tuple and I have seen tuples of length 1 and 3.
I think that for typing, we should type what we do right now. And in another PR we can decide to restrict it to 2.
Note, it was officially meant to be 2.
There was a problem hiding this comment.
#49864 implements that, will adjust here once that's in. That won't be enough to make bond pass cleanly here as it has Optionals in the identifier str tuple, but decreases the need for the workaround.
This reflects current practice, but the intent has been to have them as 2-tuples, and a future change is likely to start enforcing that (again). Refs #49670 (comment)
Proposed change
This makes entity device info a TypedDict, providing for easier coding and better typecheck time validation.
Type defs follow the docs at https://developers.home-assistant.io/docs/device_registry_index/#device-properties as well as in the device registry. In particular, entry_type is the only thing marked as optional here per the docs above.
This contains the bare minimum to implement the change and make tests pass, next step is to fix device_info signatures in the rest of the tree. I have it prepared already, but will submit separately to keep PR size at bay, unless a reviewer here requests it to be included already here.
FYI @prystupa: This exposes a "bug" in the bond component; it uses a three-tuple as device identifier, with one component of it being possibly None, whereas the expected identifiers are declared to be two-tuples of str (no optionals). I've marked it to be ignored here, that'll preserve current status. I guess the bug is currently a typecheck level only, as nothing seems to peek into the contents of those identifiers at runtime, they're just used as-is for lookup. But it would be good to get this properly fixed, preferably IMO after this PR has been merged.
Type of change
Additional information
Checklist
black --fast homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: