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

TypeVarTuple as type arg for subclasses of generic TypedDict (fixes #16975) #16977

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

erikkemperman
Copy link
Contributor

@erikkemperman erikkemperman commented Mar 1, 2024

(fixes #16975)

I'm not entirely sure about this implementation -- it fixes the issue and doesn't seem to break anything else -- but it feels like I might be doing things in the wrong phase? If so, a little nudge in the right direction would be much appreciated.

@@ -316,6 +324,7 @@ def analyze_typeddict_classdef_fields(
analyzed = self.api.anal_type(
stmt.type,
allow_required=True,
allow_unpack=True,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leaving allow_unpack out here, or setting it to False, doesn't have an effect on the test suite... Intuitively I would guess that allow_unpack should be True here but would love to understand this a little bit better.

@@ -520,6 +529,7 @@ def parse_typeddict_fields_with_types(
analyzed = self.api.anal_type(
type,
allow_required=True,
allow_unpack=True,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

And the same applies here.

Copy link
Contributor

github-actions bot commented Mar 1, 2024

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

@erikkemperman
Copy link
Contributor Author

Not wanting to appear impatient, and I appreciate that the reviewers are quite busy -- but is there anything I can do to nudge this along? Perhaps my first sentence didn't inspire confidence:

I'm not entirely sure about this implementation

For what it's worth, though, I took another long look and am now thinking this is in fact the right approach. If it helps makes things more palatable and you're unsure about the two allow_unpack arguments I commented on, I would be happy to remove those (as I mentioned, the remaining patch would still fix the concrete issue I was trying to address), or replace them with a comment.

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.

Allow TypeVarTuple as type argument for subclasses of generic TypedDict
1 participant