Use ZeroconfServiceInfo in gogogate2#59746
Conversation
|
Hey there @vangorra, @bdraco, mind taking a look at this pull request as it has been labeled with an integration ( |
|
Note: needs rebase after #59745 is merged |
6f26adc to
9ddad7c
Compare
|
Rebased now #59745 is merged |
| """Handle homekit discovery.""" | ||
| await self.async_set_unique_id(discovery_info["properties"]["id"]) | ||
| return await self._async_discovery_handler(discovery_info["host"]) | ||
| await self.async_set_unique_id(discovery_info[zeroconf.ATTR_PROPERTIES]["id"]) |
There was a problem hiding this comment.
Should we add a constant for id since its a defined field for homekit?
There was a problem hiding this comment.
id is harder to pinpoint with grep, but I had a quick scan of the components and there does seem to be a few components using it. Should I add ATTR_PROPERTIES_ID constant to zeroconf?
It's a bit long, and I could add ATTR_ID but I am worried about adding confusion with the base attributes.
There was a problem hiding this comment.
Or maybe you want me to add the constant to homekit?
There was a problem hiding this comment.
HomeKit discovery is a subset of zeroconf discovery. I think you have it in the right place.
I hate to ask for a one line change, but It really should be a separate PR.
There was a problem hiding this comment.
I thought you wanted it in this PR... I have reverted it now.
|
Note: CI failed due to a flaky test in |
|
That or ATTR_HOMEKIT_PROPERTIES_ID, ATTR_HOMEKIT_ID. Its the same regardless if its homekit or not so maybe ATTR_PROPERTIES_ID is the best choice.
Naming is hard.
… On Nov 16, 2021, at 00:37, epenet ***@***.***> wrote:
@epenet commented on this pull request.
In homeassistant/components/gogogate2/config_flow.py <#59746 (comment)>:
> """Handle homekit discovery."""
- await self.async_set_unique_id(discovery_info["properties"]["id"])
- return await self._async_discovery_handler(discovery_info["host"])
+ await self.async_set_unique_id(discovery_info[zeroconf.ATTR_PROPERTIES]["id"])
id is harder to pinpoint with grep, but I had a quick scan of the components and there does seem to be a few components using it. Should I add ATTR_PROPERTIES_ID constant to zeroconf?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#59746 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFB7CEEWI36F3TWEE6VUWDUMH34DANCNFSM5ICUZCCQ>.
|
0a6c7a1 to
ceadda2
Compare
|
Rebased to re-trigger CI following #59787 |
This reverts commit ceadda2.
|
Note: CI is failing just because of |
Proposed change
Use ZeroconfServiceInfo in gogogate2
Followup to #59745
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: