Adjust async_step_zeroconf signature for strict typing #59503
Conversation
probot-home-assistant
bot
added
code-quality
core
small-pr
|
@frenck / @bdraco I have created an architecture discussion for this as it seems the more I fix the more underlying issues it creates. Maybe we need to set all DiscoveryInfo to be fully typed before we advance on this. |
|
|
||
| return await self.async_step_confirm() | ||
|
|
||
| async_step_zeroconf = async_step_discovery |
There was a problem hiding this comment.
This line is redundant with the base class, and actually causing mypy to crash, so had to remove it!!! See linked PR (#59576)
core/homeassistant/config_entries.py
Lines 1371 to 1375 in 363de37
There was a problem hiding this comment.
Did you open in issue with mypy?
There was a problem hiding this comment.
I have not (yet) opened the issue with mypy, because I thought the line was just redundant. I will try to open an issue with them next week.
@balloob suggests that the line needs to stay there - so maybe we have to keep it and set it to ignore the type?
Or maybe there is another way to set a different signature for async_step_zeroconf (with ZeroconfServiceInfo) and async_step_discovery (DiscoveryInfoType) ?
There was a problem hiding this comment.
Ignore is fine here. The alternative would be to write out the function and await the discovery handler which would add another await only to satisfy mypy which doesn't seem like a good trade off to me.
There was a problem hiding this comment.
I didn't manage to make #type: ignore work.
If I add the line back in, the I have to remove altogether the type hint in sonos component (it seems to be the only component using DiscoveryFlowHandler with zeroconf and type hints).
Is removing the type hint on async_step_zeroconf in sonos a good (at least temporary) trade off?
There was a problem hiding this comment.
If I remove the await keyword, now pylint complains with invalid-overridden-method so I have reverted the change, but I could bring it back in with pylint disable:
def async_step_zeroconf( # pylint: disable=invalid-overridden-method
self,
discovery_info: zeroconf.ZeroconfServiceInfo,
) -> Coroutine[Any, Any, FlowResult]:
"""Handle a flow initialized by Zeroconf discovery."""
return self.async_step_discovery(cast(dict, discovery_info))
There was a problem hiding this comment.
last, but also ugly, but probably the best solution is code duplication, which solves the concern raised by @bdraco while shutting up all linters as well, allowing us to move forward:
async def async_step_zeroconf(
self, discovery_info: zeroconf.ZeroconfServiceInfo
) -> FlowResult:
"""Handle a flow initialized by Zeroconf discovery."""
if self._async_in_progress() or self._async_current_entries():
return self.async_abort(reason="single_instance_allowed")
await self.async_set_unique_id(self._domain)
return await self.async_step_confirm()There was a problem hiding this comment.
Happy to implement. @bdraco do you agree?
There was a problem hiding this comment.
Great. I have updated the code. Hopefully the CI passes and we are good to go...
epenet
requested review from
bachya,
balloob,
bdraco,
bieniu and
cgtobi
as code owners
frenck
added
the
smash
epenet
force-pushed
the
zeroconf-service-info
branch
from
fa39f52 to
4e7d366
Compare
frenck
removed
the
smash
…t#59503) Co-authored-by: epenet <epenet@users.noreply.github.com>
4 participants
Proposed change
Adjust
async_step_zeroconftype hint, as discussed in #58958 (comment)cc @frenck, @bdraco
Also see home-assistant/architecture#662
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: