Add TypeVar defaults for DataUpdateCoordinator and EntityComponent #95026

cdce8p · 2023-06-22T00:29:19Z

Proposed change

Followup to #94987

For mypy 1.4.0 I've started adding initial support for TypeVar defaults PEP 696. It's still in the early phases, however a few improvements are already possible. Basically those in this PR, nothing more yet.

For the moment, adding the TypeVar defaults for DataUpdateCoordinator and EntityComponent will only improve code completions in VS Code. E.g. where previously self.coordinator.data would only suggest Any / Unknown, it now defaults to the TypeVar default dict[str, Any].

Once the support in mypy is better, it will also help catch typing issues. I've been testing an experimental mypy build for some time now and already fixed a lot of them. E.g. #91883

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 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.

To help with the load of incoming pull requests:

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

home-assistant · 2023-06-22T00:29:26Z

Hey there @bdraco, mind taking a look at this pull request as it has been labeled with an integration (bluetooth) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of bluetooth can trigger bot actions by commenting:

@home-assistant close Closes the pull request.
@home-assistant rename Awesome new title Renames the pull request.
@home-assistant reopen Reopen the pull request.
@home-assistant unassign bluetooth Removes the current integration label and assignees on the pull request, add the integration domain after the command.

bdraco

Tested on production, no performance or adverse side effects observed.

bdraco · 2023-06-22T01:33:20Z

Thanks @cdce8p

MartinHjelmare · 2023-06-22T05:29:02Z

homeassistant/helpers/update_coordinator.py

@@ -30,12 +31,14 @@
 REQUEST_REFRESH_DEFAULT_COOLDOWN = 10
 REQUEST_REFRESH_DEFAULT_IMMEDIATE = True

-_T = TypeVar("_T")
+_DataT = TypeVar("_DataT", default=dict[str, Any])


Will this type the coordinator data as dict[str, Any] even if the implementing integration hasn't added type annotations? That would be misleading and incorrect I think.

Will this type the coordinator data as dict[str, Any] even if the implementing integration hasn't added type annotations? That would be misleading and incorrect I think.

Yes, but that's by design. A lot of times, CoordinatorEntity is used without any generic type. That causes mypy to skip all subsequent checks for self.coordinator.data as the type is assumed to be Any. In reality most integrations start out with dict[str, Any]. However, once the integration grows, the data type is often changed to a custom one like TypedDict or DataClass. Even then, the set default will be helpful. Once mypy fully supports PEP 696, it will warn that for example data doesn't have a specific attribute. There will also be a message that the type for _async_update_data doesn't match dict[str, Any]. Both of these can be used as hints to update the generic type for CoordinatorEntity / DataUpdateCoordinator.

Some examples

Airthings still uses a dict[str, Any].

core/homeassistant/components/airthings/sensor.py

Line 133 in 8987e02

class AirthingsHeaterEnergySensor(CoordinatorEntity, SensorEntity):

core/homeassistant/components/airthings/sensor.py

Lines 163 to 166 in 8987e02

@property

def native_value(self) -> StateType:

"""Return the value reported by the sensor."""

return self.coordinator.data[self._id].sensors[self.entity_description.key]

I mentioned the PR for notion earlier already #91883. Say we remove all generic arguments for CoordinatorEntity and DataUpdateCoordinator, with a future mypy version we would get these errors:

homeassistant/components/notion/__init__.py:290: error: "dict[str, Any]" has no attribute "bridges" [attr-defined] homeassistant/components/notion/__init__.py:291: error: "dict[str, Any]" has no attribute "sensors" [attr-defined] homeassistant/components/notion/__init__.py:314: error: "dict[str, Any]" has no attribute "listeners" [attr-defined] homeassistant/components/notion/__init__.py:320: error: Returning Any from function declared to return "Listener" [no-any-return] homeassistant/components/notion/__init__.py:320: error: "dict[str, Any]" has no attribute "listeners" [attr-defined] homeassistant/components/notion/__init__.py:328: error: "dict[str, Any]" has no attribute "sensors" [attr-defined] homeassistant/components/notion/__init__.py:335: error: "dict[str, Any]" has no attribute "bridges" [attr-defined] homeassistant/components/notion/__init__.py:343: error: "dict[str, Any]" has no attribute "bridges" [attr-defined] homeassistant/components/notion/__init__.py:356: error: "dict[str, Any]" has no attribute "listeners" [attr-defined] homeassistant/components/notion/sensor.py:70: error: "dict[str, Any]" has no attribute "user_preferences" [attr-defined] homeassistant/components/notion/sensor.py:72: error: "dict[str, Any]" has no attribute "user_preferences" [attr-defined] homeassistant/components/notion/diagnostics.py:43: error: "dict[str, Any]" has no attribute "asdict" [attr-defined]

Whereas without we only get one and wouldn't even know that the generic type is wrong

homeassistant/components/notion/__init__.py:320: error: Returning Any from function declared to return "Listener" [no-any-return]

--
Over the last few weeks, I've already added generic arguments to integrations which use (strict) type checking and have a custom data / DataUpdateCoordinator type. At least the issues I could find are fixed.

If we don't get any warnings of incorrect type with the current mypy version the default may just silently be wrong and misleading. I don't think we should add it until we get warnings.

If we don't get any warnings of incorrect type with the current mypy version the default may just silently be wrong and misleading. I don't think we should add it until we get warnings.

I'm working on the mypy support, hopefully the warning will be there with the release next month 🤞🏻
In the meantime, it's still an improvement as I suspect a lot / most use VS Code and Pylance which will already display the errors (if they have enabled the type checking mode).

I'm not planning to add any more defaults until then, just for these once I think the benefits outweigh the small risk of it becoming outdated.

I disagree that the benefits outweigh the problems and think we should remove this for now.

Guess we have different opinions there. Anyway you call the shots. Opened #95101

…onent (home-assistant#95026)" This reverts commit 90f5b1c.

…" (#95101) * Revert "Add TypeVar defaults for DataUpdateCoordinator and EntityComponent (#95026)" This reverts commit 90f5b1c. * Don't revert everything

Add TypeVar defaults for DataUpdateCoordinator and EntityComponent

2c54eeb

cdce8p requested review from bdraco and a team as code owners June 22, 2023 00:29

home-assistant bot added cla-signed code-quality core integration: bluetooth small-pr PRs with less than 30 lines. labels Jun 22, 2023

home-assistant bot assigned bdraco Jun 22, 2023

home-assistant bot added the Quality Scale: internal label Jun 22, 2023

bdraco approved these changes Jun 22, 2023

View reviewed changes

bdraco merged commit 90f5b1c into home-assistant:dev Jun 22, 2023
51 checks passed

cdce8p deleted the first-typevar-defaults branch June 22, 2023 01:34

MartinHjelmare reviewed Jun 22, 2023

View reviewed changes

cdce8p added a commit to cdce8p/ha-core that referenced this pull request Jun 23, 2023

Revert "Add TypeVar defaults for DataUpdateCoordinator and EntityComp…

0a1bd2a

…onent (home-assistant#95026)" This reverts commit 90f5b1c.

cdce8p mentioned this pull request Jun 23, 2023

Partial revert "Add TypeVar defaults for DataUpdateCoordinator (#95026)" #95101

Merged

20 tasks

github-actions bot locked and limited conversation to collaborators Jun 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TypeVar defaults for DataUpdateCoordinator and EntityComponent #95026

Add TypeVar defaults for DataUpdateCoordinator and EntityComponent #95026

cdce8p commented Jun 22, 2023

home-assistant bot commented Jun 22, 2023

bdraco left a comment

bdraco commented Jun 22, 2023

MartinHjelmare Jun 22, 2023 •

edited

cdce8p Jun 22, 2023

MartinHjelmare Jun 22, 2023

cdce8p Jun 22, 2023

MartinHjelmare Jun 23, 2023

cdce8p Jun 23, 2023

	@property
	def native_value(self) -> StateType:
	"""Return the value reported by the sensor."""
	return self.coordinator.data[self._id].sensors[self.entity_description.key]

Add TypeVar defaults for DataUpdateCoordinator and EntityComponent #95026

Add TypeVar defaults for DataUpdateCoordinator and EntityComponent #95026

Conversation

cdce8p commented Jun 22, 2023

Proposed change

Type of change

Additional information

Checklist

home-assistant bot commented Jun 22, 2023

bdraco left a comment

Choose a reason for hiding this comment

bdraco commented Jun 22, 2023

MartinHjelmare Jun 22, 2023 • edited

Choose a reason for hiding this comment

cdce8p Jun 22, 2023

Choose a reason for hiding this comment

Some examples

MartinHjelmare Jun 22, 2023

Choose a reason for hiding this comment

cdce8p Jun 22, 2023

Choose a reason for hiding this comment

MartinHjelmare Jun 23, 2023

Choose a reason for hiding this comment

cdce8p Jun 23, 2023

Choose a reason for hiding this comment

MartinHjelmare Jun 22, 2023 •

edited