Skip to content

use PEP 589 syntax for TypeDict#1253

Merged
TheRealFalcon merged 3 commits into
canonical:mainfrom
holmanb:holmanb/pep-589
Feb 14, 2022
Merged

use PEP 589 syntax for TypeDict#1253
TheRealFalcon merged 3 commits into
canonical:mainfrom
holmanb:holmanb/pep-589

Conversation

@holmanb
Copy link
Copy Markdown
Member

@holmanb holmanb commented Feb 9, 2022

Use PEP 589 syntax for TypeDict annotation.

Also fixes previously broken typing MetaSchema typing implementation.

Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

The old version had

if sys.version_info >= (3, 8) and hasattr(typing, "TypeDict"):

That's TypeDict rather than TypedDict, so this never actually worked.

In order for this proposed version to work, we also need to add this type to every meta definition because apparently you can't coerce a raw dict into a TypedDict. E.g., on cc_apk_configure, I currently see:
image

Can we update these meta definitions in this PR too?

Comment thread cloudinit/importer.py Outdated
)
if sys.version_info >= (3, 8):

class MetaSchema:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This needs to inherit from TypedDict.

@holmanb
Copy link
Copy Markdown
Member Author

holmanb commented Feb 14, 2022

@TheRealFalcon I rebased on the new mypy commit and it looks like the fix you suggested allows us to remove a couple of files from the "exclude" list. This is ready for re-review.

Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

Nice! LGTM.

@TheRealFalcon TheRealFalcon merged commit 50195ec into canonical:main Feb 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants