Use dataclass properties in hue discovery by epenet · Pull Request #60598 · home-assistant/core

epenet · 2021-11-30T08:28:43Z

Proposed change

Use dataclass properties in hue discovery.

Linked to #60540

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request:

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
The code has been formatted using Black (black --fast homeassistant tests)
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

No score or internal
🥈 Silver
🥇 Gold
🏆 Platinum

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

probot-home-assistant · 2021-11-30T08:28:48Z

Hey there @balloob, @marcelveldt, mind taking a look at this pull request as it has been labeled with an integration (hue) you are listed as a code owner for? Thanks!
_{^{(message by CodeOwnersMention)}}

marcelveldt

Looks good!
What about adjusting the tests ?

homeassistant/components/hue/config_flow.py

epenet · 2021-11-30T08:41:22Z

What about adjusting the tests ?

They should have been adjusted already in previous PRs: #59980, #60217, #60339
Did I miss something?

marcelveldt · 2021-11-30T08:43:32Z

What about adjusting the tests ?

They should have been adjusted already in previous PRs: #59980, #60217, #60339 Did I miss something?

Ah, sorry missed that. Was looking for the adjusted tests in this PR ;-)

marcelveldt

needs a test for the config flow condition where a discovered Host is None

epenet · 2021-11-30T09:22:18Z

The test was already there: tests/components/hue/test_config_flow.py::test_bridge_ssdp_missing_location
It was failing because str(None) generates "None" string when I expected ""empty string.

marcelveldt · 2021-11-30T09:58:24Z

homeassistant/components/hue/config_flow.py

-        host = urlparse(discovery_info[ssdp.ATTR_SSDP_LOCATION]).hostname
-        bridge = await self._get_bridge(host, discovery_info[ssdp.ATTR_UPNP_SERIAL])
+        bridge = await self._get_bridge(
+            str(host), discovery_info.upnp[ssdp.ATTR_UPNP_SERIAL]


Suggested change

str(host), discovery_info.upnp[ssdp.ATTR_UPNP_SERIAL]

host, discovery_info.upnp[ssdp.ATTR_UPNP_SERIAL]

That was my initial thought also, but hostname return str | bytes | None

huh ? why is that ?
On my setup it returns either str or None

hmm, my dev machine is on python 3.10... might be that urlparse changed between py versions...
I think this is why I added the type: ignore[arg-type] here...

Looking at the docs the namedtuple ParseResult either have a str or None value so not bytes.
https://docs.python.org/3/library/urllib.parse.html

Aha! I think I found the root cause... I have a similar issue in samsungtv PR #60595

previously discovery_info[ATTR_SSDP_LOCATION] return type was Any

now discovery_info.ssdp_location is defined as str | None

If the first argument of urlparse is Any, then urlparse.hostname returns str | None
If the first argument of urlparse is str | None, then urlparse.hostname returns str | bytes | None

I'll see if I can clean this up better.

You are 100% right. The documentation doesn't match wil reality and the type is indeed AnyStr, yikes.
Maybe add a very minor extra line that converts to string if the output is bytes ?
Or indeed leave your forced to string conversion in place.

I have reworked this, and added an extra test for an invalid hostname.

Looks good. Mypy happy now ?

Yes the tests pass. I think this is good to merge.

homeassistant/components/hue/config_flow.py

epenet · 2021-11-30T10:53:44Z

Note: this PR is required for #60561

marcelveldt

LGTM

good to merge

frenck

Nice! 👍

Use dataclass properties in hue discovery

d235cd8

epenet requested review from balloob and marcelveldt as code owners November 30, 2021 08:28

homeassistant added the cla-signed label Nov 30, 2021

probot-home-assistant bot added code-quality integration: hue small-pr PRs with less than 30 lines. labels Nov 30, 2021

probot-home-assistant bot assigned balloob and marcelveldt Nov 30, 2021

marcelveldt approved these changes Nov 30, 2021

View reviewed changes

homeassistant/components/hue/config_flow.py Outdated Show resolved Hide resolved

frenck reviewed Nov 30, 2021

View reviewed changes

homeassistant/components/hue/config_flow.py Outdated Show resolved Hide resolved

Adjust host extraction from ssdp

4ccd15e

marcelveldt requested changes Nov 30, 2021

View reviewed changes

epenet mentioned this pull request Nov 30, 2021

Hue tests are slow #60604

Closed

Fix hue test

fad0772

marcelveldt reviewed Nov 30, 2021

View reviewed changes

homeassistant/components/hue/config_flow.py Outdated Show resolved Hide resolved

epenet added 3 commits November 30, 2021 11:09

Adjust host type hint

fd254e1

Check host against None

27f0320

Adjust tests for urlparse

f64994b

marcelveldt approved these changes Nov 30, 2021

View reviewed changes

frenck approved these changes Nov 30, 2021

View reviewed changes

frenck merged commit d75785d into home-assistant:dev Nov 30, 2021

epenet deleted the hue-discovery branch November 30, 2021 15:45

github-actions bot locked and limited conversation to collaborators Dec 1, 2021

	str(host), discovery_info.upnp[ssdp.ATTR_UPNP_SERIAL]
	host, discovery_info.upnp[ssdp.ATTR_UPNP_SERIAL]

Uh oh!

Conversation

epenet commented Nov 30, 2021

Proposed change

Type of change

Additional information

Checklist

Uh oh!

probot-home-assistant bot commented Nov 30, 2021

Uh oh!

marcelveldt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

epenet commented Nov 30, 2021

Uh oh!

marcelveldt commented Nov 30, 2021

Uh oh!

marcelveldt left a comment

Choose a reason for hiding this comment

Uh oh!

epenet commented Nov 30, 2021

Uh oh!

marcelveldt Nov 30, 2021

Choose a reason for hiding this comment

Uh oh!

epenet Nov 30, 2021

Choose a reason for hiding this comment

Uh oh!

marcelveldt Nov 30, 2021 • 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!

marcelveldt Nov 30, 2021 • 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!

marcelveldt Nov 30, 2021 • 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!

epenet Nov 30, 2021

Choose a reason for hiding this comment

Uh oh!

marcelveldt Nov 30, 2021

Choose a reason for hiding this comment

Uh oh!

epenet Nov 30, 2021

Choose a reason for hiding this comment

Uh oh!

marcelveldt Nov 30, 2021

Choose a reason for hiding this comment

Uh oh!

epenet Nov 30, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

epenet commented Nov 30, 2021

Uh oh!

marcelveldt left a comment

Choose a reason for hiding this comment

Uh oh!

frenck left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

marcelveldt Nov 30, 2021 •

edited

Loading

marcelveldt Nov 30, 2021 •

edited

Loading

marcelveldt Nov 30, 2021 •

edited

Loading