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 728: TypedDict with Typed Extra Fields #3441

Merged
merged 20 commits into from
Feb 9, 2024
Merged

Conversation

PIG208
Copy link
Contributor

@PIG208 PIG208 commented Sep 18, 2023

Basic requirements (all PEP Types)

  • Read and followed PEP 1 & PEP 12
  • File created from the latest PEP template
  • PEP has next available number, & set in filename (pep-NNNN.rst), PR title (PEP 123: <Title of PEP>) and PEP header
  • Title clearly, accurately and concisely describes the content in 79 characters or less
  • Core dev/PEP editor listed as Author or Sponsor, and formally confirmed their approval
  • Author, Status (Draft), Type and Created headers filled out correctly
  • PEP-Delegate, Topic, Requires and Replaces headers completed if appropriate
  • Required sections included
    • Abstract (first section)
    • Copyright (last section; exact wording from template required)
  • Code is well-formatted (PEP 7/PEP 8) and is in code blocks, with the right lexer names if non-Python
  • PEP builds with no warnings, pre-commit checks pass and content displays as intended in the rendered HTML
  • Authors/sponsor added to .github/CODEOWNERS for the PEP

Standards Track requirements

  • PEP topic discussed in a suitable venue with general agreement that a PEP is appropriate
  • Suggested sections included (unless not applicable)
    • Motivation
    • Rationale
    • Specification
    • Backwards Compatibility
    • Security Implications
    • How to Teach This
    • Reference Implementation
    • Rejected Ideas
    • Open Issues
  • Python-Version set to valid (pre-beta) future Python version, if relevant
  • Any project stated in the PEP as supporting/endorsing/benefiting from the PEP formally confirmed such
  • Right before or after initial merging, PEP discussion thread created and linked to in Discussions-To and Post-History

📚 Documentation preview 📚: https://pep-previews--3441.org.readthedocs.build/

@PIG208 PIG208 requested a review from a team as a code owner September 18, 2023 17:42
@cpython-cla-bot
Copy link

cpython-cla-bot bot commented Sep 18, 2023

All commit authors signed the Contributor License Agreement.
CLA signed

@PIG208
Copy link
Contributor Author

PIG208 commented Sep 18, 2023

Hi @AlexWaygood! Thanks for expressing interest in this feature. Could you confirm yourself as a sponsor of this PEP?

@JelleZijlstra
Copy link
Member

There is significant overlap with some of the ideas in #3440, a proposed rewrite of PEP 705. It may be worth discussing the interaction of the two ideas on Discuss before committing to a PEP.

@PIG208
Copy link
Contributor Author

PIG208 commented Sep 18, 2023

While I assumed that literal blocks starting with :: should have Python syntax highlighting, it does not seem to be the case in the review. Not sure if I'm missing something?

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 comments.

I'll also note that I don't like this proposal's current form of using a special-cased key. I would prefer some mechanism that doesn't look like the TypedDict has a key "__extra__".

Title: TypedDict with Typed Extra Fields
Author: Zixuan James Li <[email protected]>
Sponsor: Alex Waygood <[email protected]>
Discussions-To: https://discuss.python.org/t/pep-692-using-typeddict-for-more-precise-kwargs-typing/17314
Copy link
Member

Choose a reason for hiding this comment

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

Should be a new thread, not the one for PEP 692

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before this is merged, should I create a new "Typing" thread or leave it blank until a "pep" thread can be created?

peps/pep-0728.rst Outdated Show resolved Hide resolved
peps/pep-0728.rst Show resolved Hide resolved
peps/pep-0728.rst Outdated Show resolved Hide resolved
@alicederyn
Copy link
Contributor

alicederyn commented Sep 19, 2023

Author of the PEP-705 rewrite! Excited to see this :) The syntax I'm going for in PEP-705 is

class Foo(TypedDict, other_keys=False):

I'm happy to rewrite the proposal to reference this PEP, once it is published. (I have a personal preference for other_keys=Never over __extra__ = Never, but will go with whatever the community prefers.)

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.

How would this proposal interact with the ReadOnly modifier in the PEP-705 rewrite?

@AlexWaygood
Copy link
Member

Hi @AlexWaygood! Thanks for expressing interest in this feature. Could you confirm yourself as a sponsor of this PEP?

I was assuming you'd send me a draft before making a PR to the peps repo 😄 But no worries!

In principle, I'm still happy to sponsor something along these lines, but please give me some time to read this and PEP-705 over and think about it before confirming. Ping me again if I haven't got back to you in a few days :)

@AA-Turner AA-Turner marked this pull request as draft September 19, 2023 16:38
@AA-Turner
Copy link
Member

Converting to draft to mark that there's no sponsor (yet).

A

@alicederyn
Copy link
Contributor

@AlexWaygood Did you get a chance to look at this and PEP-705 yet? I modified 705 to bring it more in line with this draft, and it's now gone out for another round of comments.

@alicederyn
Copy link
Contributor

alicederyn commented Oct 27, 2023

Eric gave some great feedback on the subset-variant of this PEP that I included in PEP-705, raising questions that need more consideration. pyright has also changed to allow type discrimination without the @final decorator. As a result, it no longer seemed appropriate to couple our PEPs, and I have deferred this completely to you.

I had a couple ideas for alternative syntax that might address Eric's points.

Ellipsis

Option 1: use an ellipsis (...) instead of __extra__. This works fine in alternative syntax and also I believe in a typescript-like inline syntax?

Foo = TypedDict("Foo", { "bar": int, ...: str })
foo: { "bar": int, ...: str }

But it is a SyntaxError in class definition syntax:

class Foo(TypedDict):
  bar: int
  ...: str  # syntax error

That leaves us deciding between:

  • a language change to allow this, which would need some serious justification,
  • this feature only working in alternative syntax, or
  • using different syntax for class definitions

I think it looks decent though, especially in inline syntax, so throwing it out there.

A type qualifier

Option 2: use a type qualifier instead of a "reserved" key name.

class Foo(TypedDict):
  bar: int
  _: OtherValues[str]

Users can use any key that isn't already used, so there's no risk of accidentally changing the meaning of existing definitions, but the PEP can recommend a conventional default to use whenever possible. I like underscore as it is short and it is already conventional to use this for "other stuff" but haven't put much thought into it.

This also has the advantage of being hyperlinked to documentation explaining what it is in editors, making it more discoverable than __extra__.

@PIG208
Copy link
Contributor Author

PIG208 commented Oct 27, 2023

I kind of like the type qualifier idea, but it can have issues with subclassing to address. Will revisit the draft probably this weekend and incorporate some new progress made from recent discussions on PEP-705.

@erictraut
Copy link
Contributor

@PIG208, what is the status of this PEP? Are you looking to move forward with it? It came up in a recent typing discussion.

@PIG208
Copy link
Contributor Author

PIG208 commented Jan 29, 2024

I need to catch up with the previous discussions on the topic to get it up-to-date. I just went back from a break and will push some updates soon.

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

I talked to @AlexWaygood and I offered to take over as the sponsor of the PEP. If that's OK with you, @PIG208, please add my name to the headers. I'll also do another review pass on the PEP once you've addressed the open feedback.

Feel free to email me (my email address is all over this repo) to discuss the logistics and process of moving the PEP forward.

@PIG208 PIG208 force-pushed the pep-728 branch 3 times, most recently from fec95c2 to 9922546 Compare February 3, 2024 23:04
Signed-off-by: Zixuan James Li <[email protected]>
Signed-off-by: Zixuan James Li <[email protected]>
Signed-off-by: Zixuan James Li <[email protected]>
I left out the rewrite to other proposed syntax such as using
"OtherFields[]" with an arbitrary key before there is sufficient
discussion on the alternatives available to us.

Signed-off-by: Zixuan James Li <[email protected]>
peps/pep-0728.rst Outdated Show resolved Hide resolved
@JelleZijlstra
Copy link
Member

The last round of changes makes some great improvements. I left a few quick comments, but will do another review and then hopefully we can merge the PEP in.

One of the next steps should be to implement the PEP in https://github.com/python/typing_extensions. I think that should just require some minor adjustments to __required_keys__ logic.

Also split parts of it into individual sections dicussing changes to
TypedDict type consistency with Mapping and dict.

Signed-off-by: Zixuan James Li <[email protected]>
Addresses some review feedback.

Signed-off-by: Zixuan James Li <[email protected]>
Signed-off-by: Zixuan James Li <[email protected]>
@PIG208
Copy link
Contributor Author

PIG208 commented Feb 8, 2024

Thank you for the review @JelleZijlstra ! Opened python/typing_extensions#329.

@PIG208
Copy link
Contributor Author

PIG208 commented Feb 8, 2024

I'm considering renaming __extra__ to __extra_items__ to reduce possible naming collisions and use item instead of field for consistency with PEP 705 and the typing spec.

This follows the style of PEP 705 and the majority of the typing spec to
use "key" or "item" instead of "field" depending on the context. It also
makes use of "value type" more to be adequately more pedantic. The
special key added is now referred to as the "reserved key" to make it
clearer that it is not intended for other purposes anymore.

It also make use of type aliases like `VT` more for brevity.

Signed-off-by: Zixuan James Li <[email protected]>
peps/pep-0728.rst Show resolved Hide resolved
peps/pep-0728.rst Outdated Show resolved Hide resolved
peps/pep-0728.rst Outdated Show resolved Hide resolved
peps/pep-0728.rst Outdated Show resolved Hide resolved
peps/pep-0728.rst Outdated Show resolved Hide resolved
peps/pep-0728.rst Outdated Show resolved Hide resolved
peps/pep-0728.rst Outdated Show resolved Hide resolved
peps/pep-0728.rst Outdated Show resolved Hide resolved
peps/pep-0728.rst Outdated Show resolved Hide resolved
peps/pep-0728.rst Outdated Show resolved Hide resolved
Signed-off-by: Zixuan James Li <[email protected]>
Signed-off-by: Zixuan James Li <[email protected]>
Signed-off-by: Zixuan James Li <[email protected]>
@PIG208
Copy link
Contributor Author

PIG208 commented Feb 9, 2024

Hmm, not sure why is .../peps/pep-0728.rst:29: WARNING: py:data reference target not found: typing.TypedDict

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

Pushing a couple of small fixes to satisfy the merge requirements, I think we're almost there!

peps/pep-0728.rst Outdated Show resolved Hide resolved
@JelleZijlstra JelleZijlstra merged commit 395a311 into python:main Feb 9, 2024
6 checks passed
@JelleZijlstra
Copy link
Member

And it's merged! It should be up at https://peps.python.org/pep-0728 momentarily. As a next step, could you post in https://discuss.python.org/c/peps/19 about the PEP, and then update the Discussions-To/Post-History headers here in another PR.

@PIG208 PIG208 deleted the pep-728 branch February 9, 2024 04:28
@PIG208
Copy link
Contributor Author

PIG208 commented Feb 9, 2024

Got it, thanks!

Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

Please could you add the Discourse link to Discussions-To and Post-History headers?

Edit: done in #3658

Python-Version: 3.13


.. highlight:: rst
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this, the code blocks should default to Python instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was wondering about why that was happening. Thanks for spotting that!


Thanks to Jelle Zijlstra for sponsoring this PEP and providing review feedback,
Eric Traut who `proposed the original design
<https://mail.python.org/archives/list/[email protected]/message/3Z72OQWVTOVS6UYUUCCII2UZN56PV5II/>`
Copy link
Member

Choose a reason for hiding this comment

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

This isn't rendering as a link properly, I think it needs a couple of underscores at the end?

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.

8 participants