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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use dataclass properties in yamaha_musiccast discovery #60749
Conversation
Hey there @vigonotion, @micha91, mind taking a look at this pull request as it has been labeled with an integration ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @epenet 馃憤
self.host = urlparse(discovery_info[ssdp.ATTR_SSDP_LOCATION]).hostname | ||
self.upnp_description = discovery_info[ssdp.ATTR_SSDP_LOCATION] | ||
self.serial_number = discovery_info.upnp[ssdp.ATTR_UPNP_SERIAL] | ||
self.host = urlparse(discovery_info.ssdp_location or "").hostname or "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean we can update the config entry host to be an empty string? Is that valid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @MartinHjelmare
This change was to satisfy mypy
:
urlparse
reacts differently if it is passedstr
versusstr | None
- then the generated
hostname
can still be None
This should be caught on line 87 though, so maybe using the form if TYPE_CHECKING
twice (once for ssdp_location
, and then again for hostname
) would have been better?
https://github.com/epenet/home-assistant-core/blob/3b0d984959beef06a29a620815de4f3e810c73e5/homeassistant/components/yamaha_musiccast/config_flow.py#L87
I think using the standard form if discovery_info.ssdp_location is None
it would have become impossible to build a test for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can refactor as part of #60787 if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the discussion around urlparse
, see this comment from a previous hue
PR: #60598 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't accommodate bugs or write code that looks like a bug to satisfy typing. Either use if TYPE_CHECKING
or some other explicit way of setting the type without allowing a potential bug to sneak in.
Proposed change
Use dataclass properties in
yamaha_musiccast
discovery.Linked to #60540
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: