Skip to content

Fix required and optional keys inheritance for TypedDict#700

Merged
gvanrossum merged 7 commits intopython:masterfrom
vemel:typed-dict-keys
Feb 12, 2020
Merged

Fix required and optional keys inheritance for TypedDict#700
gvanrossum merged 7 commits intopython:masterfrom
vemel:typed-dict-keys

Conversation

@vemel
Copy link
Copy Markdown
Contributor

@vemel vemel commented Feb 7, 2020 •
edited
Loading

I realized that the inheritance of TypedDict does not work as it should.

class _Animal(TypedDict):
     name: str

class Animal(_Animal, total=False):
     voice: str

class Cat(Animal):
     fur_color: str

I would assume that Cat should have required keys name and fur_color and optional voice.
But in reality, it will have required fur_color and optional name and voice, because Animal has total=False

Fixed

  • __required_keys__ and __optional_keys__ are copied from base classes
  • Own TypedDict annotations are added to __annotations__ after base annotations
  • Conflicting required and optional keys from base classes are removed if TypedDict redefines them

@vemel vemel requested a review from ilevkivskyi February 7, 2020 15:44
@gvanrossum
Copy link
Copy Markdown
Member

gvanrossum commented Feb 11, 2020

Should this also be updated upstream in CPython?

gvanrossum
gvanrossum reviewed Feb 11, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

LGTM except I wish to rename _Animal and get rid of tail.

Comment thread typing_extensions/src_py3/test_typing_extensions.py Outdated

class Cat(Animal):
fur_color: str
tail: int
Copy link
Copy Markdown
Member

@gvanrossum gvanrossum Feb 11, 2020

Choose a reason for hiding this comment

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

This isn't allowed by mypy (you can't change the type of an attribute in a subclass) so I recommend deleting the tail attribute altogether.

Copy link
Copy Markdown
Contributor Author

@vemel vemel Feb 11, 2020

Choose a reason for hiding this comment

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

Makes sense, deleting it. I also will not remove conflicting keys from __required_keys__ and __optional_keys__ since conflicts are not allowed.

Copy link
Copy Markdown
Contributor Author

@vemel vemel Feb 11, 2020

Choose a reason for hiding this comment

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

Pushed changes, updated tests.

Comment thread typing_extensions/src_py3/test_typing_extensions.py Outdated
log_level: int
log_path: str

class _Animal(TypedDict):
Copy link
Copy Markdown
Member

@gvanrossum gvanrossum Feb 11, 2020

Choose a reason for hiding this comment

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

I don't see a reason for this class name to start with _ -- maybe rename to BaseAnimal?

Copy link
Copy Markdown
Contributor Author

@vemel vemel Feb 11, 2020

Choose a reason for hiding this comment

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

I am not aware of a convention how to name TypedDict that has both required and optional keys. Usually I prefix a TypedDict with required keys with an underscore.

Copy link
Copy Markdown
Contributor Author

@vemel vemel Feb 11, 2020

Choose a reason for hiding this comment

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

Renamed to BaseAnimal.

@vemel
Copy link
Copy Markdown
Contributor Author

vemel commented Feb 11, 2020

Travis CI build failed for Python 3.5.1, but it looks unrelated.

  File "/home/travis/virtualenv/python3.5.1/lib/python3.5/site-packages/importlib_metadata/__init__.py", line 9, in <module>
    import zipp
ImportError: No module named 'zipp'

Probably restarting Travis build will help.

@vemel vemel requested a review from gvanrossum February 11, 2020 23:13
gvanrossum
gvanrossum approved these changes Feb 12, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Thanks! I will land this despite the 3.5.1 failure.

@gvanrossum gvanrossum merged commit e796957 into python:master Feb 12, 2020
gvanrossum pushed a commit to python/cpython that referenced this pull request Feb 13, 2020
@vemel
bpo-39627: Fix TypedDict totality check for inherited keys (#18503)
10e87e5 
(Adapted from python/typing#700)
@apadmarao apadmarao mentioned this pull request Mar 27, 2020
Closed
@Zac-HD
Copy link
Copy Markdown
Contributor

Zac-HD commented May 4, 2020

@vemel - I think this should go upstream, as a follow-up to python/cpython#17214, since CPython is still using the pre-this-PR logic.

@gvanrossum
Copy link
Copy Markdown
Member

gvanrossum commented May 4, 2020 via email

@domdfcoding
Copy link
Copy Markdown
Contributor

domdfcoding commented Sep 11, 2020

@Zac-HD Did you submit a PR to CPython for this? Having trouble finding it.

@Zac-HD
Copy link
Copy Markdown
Contributor

Zac-HD commented Sep 11, 2020

According to https://github.com/python/cpython/pulls?q=is%3Apr+author%3AZac-HD I didn't, and remembering what May was like I believe it. If you'd like to, go for it 🙂

JelleZijlstra pushed a commit to python/typing_extensions that referenced this pull request May 19, 2022
@vemel
Fix required and optional keys inheritance for TypedDict (#700)
7d04c95 
(For a complete description of the issue see python/typing#700.)
JelleZijlstra pushed a commit to python/typing_extensions that referenced this pull request May 19, 2022
@vemel
Fix required and optional keys inheritance for TypedDict (#700)
5a02150 
(For a complete description of the issue see python/typing#700.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gvanrossum gvanrossum gvanrossum approved these changes

@ilevkivskyi ilevkivskyi Awaiting requested review from ilevkivskyi

Assignees

No one assigned

Labels

CLA signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@vemel @gvanrossum @Zac-HD @domdfcoding @the-knights-who-say-ni