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

PEP 705: Rewrite to focus on TypedDict changes #3440

Merged
merged 22 commits into from
Oct 16, 2023

Conversation

alicederyn
Copy link
Contributor

@alicederyn alicederyn commented Sep 18, 2023

  • Change is either:
    • To a Draft PEP
    • To an Accepted or Final PEP, with Steering Council approval
    • To fix an editorial issue (markup, typo, link, header, etc)
  • PR title prefixed with PEP number (e.g. 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.

Copy link
Member

@JelleZijlstra JelleZijlstra left a 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.

peps/pep-0705.rst Outdated Show resolved Hide resolved
peps/pep-0705.rst Outdated Show resolved Hide resolved
peps/pep-0705.rst Outdated Show resolved Hide resolved
peps/pep-0705.rst Outdated Show resolved Hide resolved
peps/pep-0705.rst Outdated Show resolved Hide resolved
peps/pep-0705.rst Outdated Show resolved Hide resolved
peps/pep-0705.rst Outdated Show resolved Hide resolved
peps/pep-0705.rst Show resolved Hide resolved
peps/pep-0705.rst Show resolved Hide resolved
@JelleZijlstra
Copy link
Member

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 __readonly_keys__, __mutable_keys__, __readonly__, and __other_keys__.

A few other notes:

  • This PR specifies that it is an error to have both readonly=True and ReadOnly[] on a key. However, PEP 655 accepts the analogous construct (total=False + NotRequired[]). It might be nicer to allow it here too.
  • Should a TypedDict that inherits from an other_keys=False TypedDict be required to also set other_keys=False explicitly?

@PIG208
Copy link
Contributor

PIG208 commented Sep 18, 2023

Once the new draft is merged, should we repurpose the original discussion on TypedMapping, or should a new post be created? Either way, we can perhaps discuss the extra fields/other_keys design there.

@JelleZijlstra
Copy link
Member

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 __extra__/other_keys since this PEP covers a lot more ground.

alicederyn and others added 2 commits September 19, 2023 08:13
@alicederyn
Copy link
Contributor Author

I'll do the implementation details from python/typing_extensions#284 as a follow-up PR.

@alicederyn
Copy link
Contributor Author

This PR specifies that it is an error to have both readonly=True and ReadOnly[] on a key. However, PEP 655 accepts the analogous construct (total=False + NotRequired[]). It might be nicer to allow it here too.

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 readonly=True.

I'm not sure what advantage respecifying ReadOnly might have to counterbalance this?

peps/pep-0705.rst Outdated Show resolved Hide resolved
peps/pep-0705.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@PIG208 PIG208 left a 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.

peps/pep-0705.rst Show resolved Hide resolved
peps/pep-0705.rst Show resolved Hide resolved
peps/pep-0705.rst Show resolved Hide resolved
peps/pep-0705.rst Outdated Show resolved Hide resolved
peps/pep-0705.rst Outdated Show resolved Hide resolved
@PIG208
Copy link
Contributor

PIG208 commented Sep 21, 2023

My main concern that led me to add other_keys to PEP-705 is that declaring a TypedDict as @final to indicate other keys won't be present no longer semantically makes sense if keys can be read-only, so we need an officially sanctioned approach.

Just read this comment that explains the motivation for adding other_keys and the discussion on @final. I'm not sure if I understand why read-only keys make a @final annotated type not final anymore? Is it because the user is allowed to refine the types of read-only keys?

@JelleZijlstra
Copy link
Member

Just read this comment that explains the motivation for adding other_keys and the discussion on @final. I'm not sure if I understand why read-only keys make a @final annotated type not final anymore? Is it because the user is allowed to refine the types of read-only keys?

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]

@PIG208
Copy link
Contributor

PIG208 commented Sep 22, 2023

I found microsoft/pyright#5254 to be helpful in understanding the issue with @final with a bit more context.

@alicederyn
Copy link
Contributor Author

alicederyn commented Sep 22, 2023

it defaults extra keys' type to ReadOnly[NotRequired[Any]]

This is supposed to exactly match the existing behaviour of a TypedDict, i.e. with no other_keys/__extra__ set:

  • d[k] will be flagged as an error as k may not be present (NotRequired)
  • d.get(k) is fine, but the returned type is unknown
  • d[k] = ... will be flagged as an error as k is readonly

It seems as if PEP-728 is implicitly making all "extra" keys non-read-only. It would probably be useful to users to be able to use ReadOnly there too:

  • subclasses could declare new required keys
  • subclasses could refine the type of the extra keys

Copy link
Member

@JelleZijlstra JelleZijlstra left a 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.

peps/pep-0705.rst Outdated Show resolved Hide resolved
peps/pep-0705.rst Show resolved Hide resolved
Make it clear that the new consistency rules should match the existing rules for any types not using the new features.
@alicederyn
Copy link
Contributor Author

alicederyn commented Sep 23, 2023

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 other_keys=Never, so have modified the proposal accordingly, and made multiple notes in the "rejected ideas" section, as well as linking the PEP 728 PR. I will update the PEP to reflect changes in PEP 728 as they arise.

Copy link
Member

@JelleZijlstra JelleZijlstra left a 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.

Copy link
Member

@pablogsal pablogsal left a 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!

@JelleZijlstra JelleZijlstra merged commit 54f10d9 into python:main Oct 16, 2023
6 checks passed
@alicederyn alicederyn deleted the readonly.keys branch October 17, 2023 12:38
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.

None yet

5 participants