Add strict typing to ring integration #115276

sdb9696 · 2024-04-09T10:07:44Z

Proposed change

Add ring integration to .strict-typing and associated type hint changes.

Most changes are either minor type hints or using the ring_doorbell library typed get functions.
sensor.py has been refactored to support the value_fn for native values.
The RingDeviceData class has been dropped from the coordinator as the library now stores history with the device.

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)
Deprecation (breaking change to happen in the future)
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
I have followed the perfect PR recommendations
The code has been formatted using Ruff (ruff format 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.

To help with the load of incoming pull requests:

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

homeassistant/components/ring/camera.py

joostlek · 2024-04-09T10:18:32Z

homeassistant/components/ring/light.py

+    ON = auto()
+    OFF = auto()


What's this?

The StrEnum was so that I could type hint the possible state values. The auto() sets the string value to the lower case value of the member name.

joostlek · 2024-04-09T10:22:51Z

tests/components/ring/common.py

@@ -15,4 +15,4 @@ async def setup_platform(hass, platform):
    )
    with patch("homeassistant.components.ring.PLATFORMS", [platform]):
        assert await async_setup_component(hass, DOMAIN, {})
-    await hass.async_block_till_done()
+    await hass.async_block_till_done(wait_background_tasks=True)


Why is this change here

I had a test failing after I added the unique_id migration code in a separate PR and I added it as per this PR #112726 suggesting it should be in place for tests. In the end the failure was something else so I could pull it out but it seems useful to have it there.

cdce8p

Thanks a lot @sdb9696! Took some time to go through the changes and left some comments to make the annotations even better. Please let me know if you have any questions.

homeassistant/components/ring/__init__.py

homeassistant/components/ring/binary_sensor.py

homeassistant/components/ring/sensor.py

home-assistant · 2024-04-09T22:04:49Z

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

bdraco · 2024-04-09T22:21:24Z

Merged in dev to see if CI is fixed

sdb9696 · 2024-04-10T09:57:40Z

Many thanks for the excellent review @cdce8p, some really great suggestions!

I've implemented them all except I couldn't get the default TypeVar working properly as per my comments above.

homeassistant/components/ring/__init__.py

homeassistant/components/ring/binary_sensor.py

sdb9696 · 2024-04-10T11:07:15Z

Removed the prefixes. Any thoughts on the default TypeVar not working or should I just leave it as-is?

cdce8p

Left a few more suggestions to get the TypeVar default working. It can be a bit tricky sometimes.

See also my other comments regarding your questions on my earlier comments.

homeassistant/components/ring/binary_sensor.py

homeassistant/components/ring/sensor.py

cdce8p · 2024-04-10T10:59:36Z

homeassistant/components/ring/sensor.py

@@ -59,183 +62,170 @@ async def async_setup_entry(
 class RingSensor(RingEntity, SensorEntity):
    """A sensor implementation for Ring device."""

-    entity_description: RingSensorEntityDescription
+    entity_description: RingSensorEntityDescription[RingGeneric]


Suggested change

entity_description: RingSensorEntityDescription[RingGeneric]

entity_description: RingSensorEntityDescription[_RingGenericT]

_device: _RingGenericT

Ideally _device in RingBaseEntity would already be generic but that will likely break a few more things. If you want to explore that, I suggest to do it afterwards. For now setting _device: _RingGenericT here works fine.

Feel free to ping me once you do 😄

That's done, I previously tried to get the _device generic in RingBaseEntity but ran into a few issues. I've reached out on discord to ask about this.

homeassistant/components/ring/sensor.py

cdce8p · 2024-04-10T11:00:42Z

homeassistant/components/ring/sensor.py

+        self._device = self._get_coordinator_data().get_device(
+            self._device.device_api_id
+        )


Suggested change

self._device = self._get_coordinator_data().get_device(

self._device.device_api_id

)

self._device = cast(

_RingGenericT,

self._get_coordinator_data().get_device(self._device.device_api_id),

)

This is a limitation from the library. get_device always returns RingGeneric. Ideally it would return the correct subclass for a specific device_api_id.

Possible, but that is advanced typing territory. Message me on discord if you're interested. Then we can see if we're able to get this to work together.

Am very interested, have reached out.

homeassistant/components/ring/sensor.py

cdce8p · 2024-04-10T22:01:52Z

homeassistant/components/ring/sensor.py

+SENSOR_TYPES: tuple[
+    RingSensorEntityDescription[RingGeneric]
+    | RingSensorEntityDescription[RingDoorBell | RingChime]
+    | RingSensorEntityDescription[RingOther],
+    ...,
+] = (


Suggested change

SENSOR_TYPES: tuple[

RingSensorEntityDescription[RingGeneric]

| RingSensorEntityDescription[RingDoorBell | RingChime]

| RingSensorEntityDescription[RingOther],

...,

] = (

SENSOR_TYPES: tuple[RingSensorEntityDescription[Any, ...] = (

Quite a tricky situation here. This actually needs to be Any, ... for the RingSensor(..., description) call to be correct. As you noticed though, it causes the inference for value_fn to break. Not sure there is a good solution here.

It doesn't work correctly with the current annotations either. Just an example, for the battery description, device inside the value_fn lambda would be inferred as RingDoorBell | RingChime by both mypy and pyright. I honestly don't know why.

Anyway, we somehow have to make it work now. I would suggest using Any but then for each RingSensorEntityDescription adding a subscript even if it should have been provided by the TypeVar default. That should do.

Ah yes, so it did, that's strange. Anyway I've added the subscripts, reverted to Any and put a comment to explain.

homeassistant/components/ring/sensor.py

homeassistant/components/ring/camera.py

cdce8p

Awesome, thanks @sdb9696 👍🏻

Add strict typing to ring integration

e9737dd

home-assistant bot added cla-signed code-quality has-tests integration: ring by-code-owner Quality Scale: silver labels Apr 9, 2024

joostlek reviewed Apr 9, 2024

View reviewed changes

sdb9696 added 2 commits April 9, 2024 11:42

Do not overwrite aged history values

4d681a0

Update camera extra state attrs to Mapping

d9646bc

cdce8p requested changes Apr 9, 2024

View reviewed changes

home-assistant bot marked this pull request as draft April 9, 2024 22:04

Merge branch 'dev' into update_ring_typing

81a74db

Update typing post review

3d1613d

sdb9696 marked this pull request as ready for review April 10, 2024 09:57

home-assistant bot requested a review from cdce8p April 10, 2024 09:57

cdce8p reviewed Apr 10, 2024

View reviewed changes

homeassistant/components/ring/__init__.py Outdated Show resolved Hide resolved

homeassistant/components/ring/binary_sensor.py Outdated Show resolved Hide resolved

Remove ring prefix from RingData attributes

ac564b8

sdb9696 requested a review from cdce8p April 10, 2024 11:07

cdce8p requested changes Apr 10, 2024

View reviewed changes

home-assistant bot marked this pull request as draft April 10, 2024 11:09

Use TypeVar default and a few other post review suggestions

622601d

sdb9696 marked this pull request as ready for review April 10, 2024 11:52

home-assistant bot requested a review from cdce8p April 10, 2024 11:52

cdce8p requested changes Apr 10, 2024

View reviewed changes

home-assistant bot marked this pull request as draft April 10, 2024 22:01

cdce8p reviewed Apr 10, 2024

View reviewed changes

homeassistant/components/ring/sensor.py Outdated Show resolved Hide resolved

cdce8p reviewed Apr 10, 2024

View reviewed changes

homeassistant/components/ring/camera.py Outdated Show resolved Hide resolved

sdb9696 and others added 2 commits April 11, 2024 08:43

Re-add Any to tuple definition and other tweaks

cdc96ad

Merge branch 'dev' into update_ring_typing

9e5f51a

sdb9696 marked this pull request as ready for review April 11, 2024 07:54

home-assistant bot requested a review from cdce8p April 11, 2024 07:54

cdce8p approved these changes Apr 11, 2024

View reviewed changes

cdce8p merged commit 6954fcc into home-assistant:dev Apr 11, 2024
24 checks passed

github-actions bot locked and limited conversation to collaborators Apr 12, 2024

sdb9696 deleted the update_ring_typing branch April 19, 2024 10:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add strict typing to ring integration #115276

Add strict typing to ring integration #115276

sdb9696 commented Apr 9, 2024

joostlek Apr 9, 2024

sdb9696 Apr 9, 2024

joostlek Apr 9, 2024

sdb9696 Apr 9, 2024

cdce8p left a comment

home-assistant bot commented Apr 9, 2024

bdraco commented Apr 9, 2024

sdb9696 commented Apr 10, 2024

sdb9696 commented Apr 10, 2024

cdce8p left a comment

cdce8p Apr 10, 2024

sdb9696 Apr 10, 2024

cdce8p Apr 10, 2024

sdb9696 Apr 10, 2024

cdce8p Apr 10, 2024

sdb9696 Apr 11, 2024

cdce8p left a comment

	entity_description: RingSensorEntityDescription[RingGeneric]
	entity_description: RingSensorEntityDescription[_RingGenericT]
	_device: _RingGenericT

-        self._device = self._get_coordinator_data().get_device(
-            self._device.device_api_id
-        )
+        self._device = cast(
+            _RingGenericT,
+            self._get_coordinator_data().get_device(self._device.device_api_id),
+        )

Add strict typing to ring integration #115276

Add strict typing to ring integration #115276

Conversation

sdb9696 commented Apr 9, 2024

Proposed change

Type of change

Additional information

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdce8p left a comment

Choose a reason for hiding this comment

home-assistant bot commented Apr 9, 2024

bdraco commented Apr 9, 2024

sdb9696 commented Apr 10, 2024

sdb9696 commented Apr 10, 2024

cdce8p left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdce8p left a comment

Choose a reason for hiding this comment