Fix typing for wemo by esev · Pull Request #62157 · home-assistant/core

esev · 2021-12-17T03:58:40Z

Proposed change

Fix & enable type checking for the wemo component.

These type checks, in wemo_device.py, uncovered an mistake with passing an Awaitable to hass.add_job rather than to hass.create_task. Fixed in #62159. As noted in the comment below, though, this wouldn't have caused an issue.

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.

bdraco · 2021-12-19T05:45:10Z

Also fixed a bug with passing an Awaitable to hass.add_job rather than to hass.create_task, caught by the type checks, in wemo_device.py.

Its probably actually going to reach async_create_task because of https://github.com/home-assistant/core/blob/dev/homeassistant/core.py#L356

hass.create_task is better though since it is less ambiguous and just a touch faster

bdraco · 2021-12-19T05:45:21Z

This one has a conflict

esev · 2021-12-19T06:04:06Z

This one has a conflict

Rebased.

bdraco · 2021-12-19T07:38:43Z


 _LOGGER = logging.getLogger(__name__)

+HostPortTuple = Tuple[str, Optional[str]]


Is the port really a string?

No it is not. Nice catch, thank you! I missed the cv.port() call. Fixed.

bdraco · 2021-12-19T07:40:30Z

-    def current_power_w(self):
+    def current_power_w(self) -> float | None:
        """Return the current power usage in W."""
        if isinstance(self.wemo, Insight):


Would save some indent if you reversed the condition and return None in the indent, then outdent below

Thanks! Rearranged this method and the one below.

Co-authored-by: J. Nick Koston <nick@koston.org>

bdraco · 2021-12-19T08:14:57Z

 """Support for WeMo device discovery."""
 from __future__ import annotations

+from collections.abc import MutableSet, Sequence


MutableSet unused now?

Yes. You're quick! I didn't get e92b075 pushed fast enough. :)

bdraco · 2021-12-19T09:16:32Z

Will take another look tomorrow. 3am here so sleep time

bdraco · 2021-12-19T23:54:20Z

-    async def discover_statics(self):
+    async def discover_statics(self) -> None:
        """Initialize or Re-Initialize connections to statically configured devices."""
        if self._static_config:


If you reverse this condition and return, you can outdent below

Done. 96d9573

bdraco · 2021-12-19T23:57:41Z

            if standby_state == WEMO_STANDBY:
                return STATE_STANDBY
            return STATE_UNKNOWN
+        return ""


Suggested change

return ""

return ""

I think you can assert False since this should be unreachable

Thank you. Done. 21f3ea8

frenck

Left a couple of comments, but could leave quite a few more of these. Why are we converting types so much in this PR?

frenck · 2021-12-20T07:26:14Z

    def name(self) -> str:
        """Return the name of the device if any."""
-        return self.light.name
+        return str(self.light.name)


This seems odd, why do we convert it to a string here? Is it an int?

frenck · 2021-12-20T07:26:24Z

+    def unique_id(self) -> str:
        """Return the ID of this light."""
-        return self.light.uniqueID
+        return str(self.light.uniqueID)


Same as above

frenck · 2021-12-20T07:26:42Z

+    def brightness(self) -> int:
        """Return the brightness of this light between 0..255."""
-        return self.light.state.get("level", 255)
+        return int(self.light.state.get("level", 255))


Why do we convert it here? Is it not an int?

frenck · 2021-12-20T07:27:02Z

        """Return the color temperature of this light in mireds."""
-        return self.light.state.get("temperature_mireds")
+        if (temp := self.light.state.get("temperature_mireds")) is not None:
+            return int(temp)


Wasn't this an int already?

bdraco · 2021-12-20T09:25:21Z

I'm pretty sure this is because it looks like pywemo hasn't implemented PEP 561 yet

frenck · 2021-12-20T09:30:32Z

But in that case, we should cast, not convert...
so if that is the case, please adjust this in a new PR

esev · 2021-12-20T15:16:28Z

Thanks for catching that. I'll send a new PR to correct this tonight after work.

homeassistant added the cla-signed label Dec 17, 2021

probot-home-assistant Bot added code-quality integration: wemo by-code-owner labels Dec 17, 2021

esev force-pushed the wemo_strict_typing branch from 1ce1598 to 1a70e8b Compare December 17, 2021 04:05

esev marked this pull request as ready for review December 17, 2021 04:23

esev marked this pull request as draft December 17, 2021 04:36

This was referenced Dec 17, 2021

Add wemo config_flow test to get 100% coverage #62158

Merged

Fix Wemo create task for awaitable #62159

Merged

Fix typing for wemo

519ca6c

esev force-pushed the wemo_strict_typing branch from 8525c18 to 519ca6c Compare December 19, 2021 05:59

esev marked this pull request as ready for review December 19, 2021 06:03

bdraco reviewed Dec 19, 2021

View reviewed changes

Comment thread homeassistant/components/wemo/__init__.py Outdated

bdraco reviewed Dec 19, 2021

View reviewed changes

esev and others added 3 commits December 18, 2021 23:56

Avoid extra indent in energy/power properties

0866a33

HostPortTuple port is an int not str

2c6a51c

Use set rather than MutableSet

df897a0

Co-authored-by: J. Nick Koston <nick@koston.org>

bdraco self-requested a review December 19, 2021 08:13

MutableSet import is not longer needed

e92b075

bdraco reviewed Dec 19, 2021

View reviewed changes

Test color_temp

2a5c9c2

Rename watts -> milliwatts

cca69dd

bdraco reviewed Dec 19, 2021

View reviewed changes

Rearrange discover_statics to allow outdenting code

96d9573

User "assert False" to annotate unreachable code

21f3ea8

bdraco approved these changes Dec 20, 2021

View reviewed changes

bdraco merged commit 1318597 into home-assistant:dev Dec 20, 2021

esev deleted the wemo_strict_typing branch December 20, 2021 00:09

frenck reviewed Dec 20, 2021

View reviewed changes

esev mentioned this pull request Dec 21, 2021

Cast types in wemo rather than converting #62454

Merged

22 tasks

github-actions Bot locked and limited conversation to collaborators Dec 21, 2021


		_LOGGER = logging.getLogger(__name__)

		HostPortTuple = Tuple[str, Optional[str]]

Uh oh!

Conversation

esev commented Dec 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed change

Type of change

Additional information

Checklist

Uh oh!

bdraco commented Dec 19, 2021

Uh oh!

bdraco commented Dec 19, 2021

Uh oh!

esev commented Dec 19, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

esev Dec 19, 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!

bdraco commented Dec 19, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frenck left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bdraco commented Dec 20, 2021

Uh oh!

frenck commented Dec 20, 2021

Uh oh!

esev commented Dec 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

esev commented Dec 17, 2021 •

edited

Loading

esev Dec 19, 2021 •

edited

Loading