Skip to content
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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Mapping[str, Any] instead of dict in Entity #48421

Merged
merged 3 commits into from
Mar 31, 2021

Conversation

KapJI
Copy link
Member

@KapJI KapJI commented Mar 27, 2021

Proposed change

This will allow components to use strictly typed TypedDict for return values of these methods. Mapping is covariant and TypedDict is compatible with Mapping[str, Any] but not with dict[str, Any].

There was discussion in mypy about if TypedDict should be compatible withdict[str, Any] and the outcome of that discussion is that Mapping should be used instead: python/mypy#4976

The idea is to make it possible to annotate components with types without using Any. So, for example, overrided device_info in component can return TypedDict instead of dict[str, Any]. This will allow making components a bit safer.

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:

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

@frenck
Copy link
Member

frenck commented Mar 27, 2021

I am confused, the consumer of these return values is Home Assistant, which doesn't expect any typedict bound result. What would it add?

Edit: nevermind, got it.

@KapJI
Copy link
Member Author

KapJI commented Mar 27, 2021

Yeah, it's for components which produce these values not for consumers.

@KapJI KapJI requested a review from frenck March 29, 2021 17:15
@KapJI
Copy link
Member Author

KapJI commented Mar 30, 2021

@frenck do you have any other concerns about this?

Copy link
Member

@frenck frenck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with this change, but I do want a second opinion on that (as I expect this to be happing in more places over time if we accept this).

@frenck frenck added the second-opinion-wanted Add this label when a reviewer needs a second opinion from another member. label Mar 30, 2021
@KapJI
Copy link
Member Author

KapJI commented Mar 30, 2021

Btw, typing documentation suggests using Mapping for arguments instead of Dict: https://docs.python.org/3/library/typing.html#typing.Dict

@KapJI
Copy link
Member Author

KapJI commented Mar 30, 2021

Also it says that typing.Mapping is going to be deprecated and with PEP 585 collections.abc.Mapping should be used instead: https://docs.python.org/3/library/typing.html#typing.Mapping

I can make this change.

@KapJI
Copy link
Member Author

KapJI commented Mar 30, 2021

@cdce8p I saw you were working on changing type hints to modern ones from PEP 585. Maybe you'll be also interested in migrating typing.Mapping and similar collections.

@cdce8p
Copy link
Member

cdce8p commented Mar 30, 2021

@KapJI Technically you're right. However changing imports and figuring out if the typing one can safely be removed isn't as easy as with builtins. Currently, we also don't have a good enforcement mechanism so we need to go through it manually from time to time. I figured that it's probably not worth it to do it now. 3.9 will be the default by this time next year and there is still plenty time until they are eventually removed.

You're, of cause, free to start using them today.

Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will work! It just depends on if the results are going to be change later. Since this isn't the case Mapping will allow both TypedDict and dict as return values.

@frenck I just think state_attributes should probably be updated as well. It might not be overwritten by the individual integration but will be by the component base class.

@KapJI
Copy link
Member Author

KapJI commented Mar 31, 2021

@frenck is it good to go now?

Should I update state_attributes as well?

@frenck
Copy link
Member

frenck commented Mar 31, 2021

It might not be overwritten by the individual integration but will be by the component base class.

In that case, it can still not be overridden and thus types, as even the individual component overriding it, doesn't know what the upstream integration will bring in.

@KapJI
Copy link
Member Author

KapJI commented Mar 31, 2021

Ok, let's keep it as is.

@frenck frenck merged commit a6759d7 into home-assistant:dev Mar 31, 2021
@KapJI KapJI deleted the use-mapping branch March 31, 2021 17:35
@KapJI
Copy link
Member Author

KapJI commented Mar 31, 2021

Thanks, this will help to use stricter type checks in our component!

@github-actions github-actions bot locked and limited conversation to collaborators Apr 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-signed code-quality core second-opinion-wanted Add this label when a reviewer needs a second opinion from another member. small-pr PRs with less than 30 lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants