Skip to content

Fix required and optional keys inheritance for TypedDict - #700

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

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

Conversation

@vemel

@vemel vemel commented Feb 7, 2020

Copy link
Copy Markdown
Contributor

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

Should this also be updated upstream in CPython?

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

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.


class Cat(Animal):
fur_color: str
tail: int

Copy link
Copy Markdown
Member

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

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

Choose a reason for hiding this comment

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

Pushed changes, updated tests.

log_level: int
log_path: str

class _Animal(TypedDict):

Copy link
Copy Markdown
Member

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

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

Choose a reason for hiding this comment

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

Renamed to BaseAnimal.

@vemel

vemel commented Feb 11, 2020

Copy link
Copy Markdown
Contributor Author

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 left a comment

Copy link
Copy Markdown
Member

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
@Zac-HD

Zac-HD commented May 4, 2020

Copy link
Copy Markdown
Contributor

@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

gvanrossum commented May 4, 2020 via email

Copy link
Copy Markdown
Member

@domdfcoding

Copy link
Copy Markdown
Contributor

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

@Zac-HD

Zac-HD commented Sep 11, 2020

Copy link
Copy Markdown
Contributor

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
JelleZijlstra pushed a commit to python/typing_extensions that referenced this pull request May 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants