-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
PEP 705: Rewrite to focus on TypedDict changes #3440
Conversation
1f818d7
to
4d9cc0a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great. I have a few minor wording suggestions and two more substantive comments.
I'm also going to try implementing this in typing-extensions
.
I wrote a draft runtime implementation at python/typing_extensions#284. Please take a look to see if you agree with the choices I made. It would also be good to mention some of the runtime implementation details in the PEP, like the new attributes A few other notes:
|
Once the new draft is merged, should we repurpose the original discussion on |
It probably makes sense to start a new thread since the new version of the PEP is so different. I would prefer a separate thread about |
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
I'll do the implementation details from python/typing_extensions#284 as a follow-up PR. |
I was thinking that it would be important to avoid confusing examples like: class Foo(TypedDict, readonly=True):
key1: ReadOnly[str]
key2: int Skimming this, it would be easy for someone to assume key2 was not readonly, missing the I'm not sure what advantage respecifying |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for all the work! I just went through the proposal, and I think the interaction of other_keys=False
with other parts of the type system is quite consistent with the draft PEP for __extra__
, corresponding to the __extra__: Never
case.
While reading this, I feel that other_key
sort of relies on readonly
(because it defaults extra keys' type to ReadOnly[NotRequired[Any]]
), but in other cases, they don't seem to interact with each other a lot.
I'm not sure if having other_key
is a requirement for implementing readonly
. Potentially, we can limit the scope of PEP 705 to readonly
to make it simpler and work on type discrimination on the other PEP. Would be interested in hearing more thoughts on this matter.
Just read this comment that explains the motivation for adding |
Yes, that's right. So you could have this: class TD1(TypedDict, other_keys=False):
a: ReadOnly[int | None] And it would be legal to have a subclass: class TD2(TD1, other_keys=False):
a: ReadOnly[int] |
I found microsoft/pyright#5254 to be helpful in understanding the issue with |
This is supposed to exactly match the existing behaviour of a TypedDict, i.e. with no
It seems as if
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few small suggestions but this generally looks good, though we still need coordination with PEP 728 and there's a few unresolved threads.
Make it clear that the new consistency rules should match the existing rules for any types not using the new features.
3dc4d20
to
554f887
Compare
Thanks, @JelleZijlstra. I believe I have now resolved all discussion threads? Though of course the text changes may raise more 😄 I think the consensus above re PEP 728 was to go with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! @pablogsal as the sponsor should approve too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thank you everyone for the fantastic work!
PEP 123: Summary of changes
)Feedback on mailing list was to narrow the focus of the PEP to just modifying TypedDict, as that covers all the use-cases we were interested in. If separate use-cases for a separate TypedMapping protocol type come up, they can form the foundation of a separate PEP.