8000 PEP 705: TypedMapping by alicederyn · Pull Request #2997 · python/peps · GitHub
[go: up one dir, main page]

Skip to content

PEP 705: TypedMapping #2997

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

Merged
merged 20 commits into from
Mar 14, 2023
Merged

PEP 705: TypedMapping #2997

merged 20 commits into from
Mar 14, 2023

Conversation

alicederyn
Copy link
Contributor

Proposal to add a TypedMapping protocol to the language.

Sponsor: @pablogsal

@alicederyn alicederyn requested a review from a team as a code owner February 1, 2023 15:26
@ghost
Copy link
ghost commented Feb 1, 2023

All commit authors signed the Contributor License Agreement.
CLA signed

@pablogsal
Copy link
Member

I confirm I am sponsoring this PEP 👍

@hugovk
Copy link
Member
hugovk commented Feb 1, 2023

Please use PEP number 705.

(Re-allocating from #2988 which was closed because it didn't yet find a sponsor.)

@alicederyn alicederyn changed the title PEP xxxx: TypedMapping PEP 705: TypedMapping Feb 1, 2023
@CAM-Gerlach CAM-Gerlach added the new-pep A new draft PEP submitted for initial review label Feb 2, 2023
Copy link
Member
@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

Standard reminder: You can directly apply all the suggestions you want in one go with Files changed -> Add to batch -> Commit

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 [resolved in suggestion]
  • Title clearly, accurately and concisely describes the content in 79 characters or less
  • Author, Status (Draft), Type and Created headers filled out correctly
  • PEP-Delegate, Topic, Requires and Replaces headers completed if appropriate [reso 8000 lved in suggestion]
  • Core dev/PEP editor listed as author or sponsor, and formally confirmed their approval
  • 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 [resolved in suggestion]
  • 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
  • Python-Version set to valid (pre-beta) future Python version
  • Any project stated in the PEP as supporting/endorsing/benefiting from it confirms such N/A
  • Right before or after initial merging, PEP discussion thread created and linked to in Discussions-To and Post-History

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 quite good and makes a convincing case for the feature, but I have a few comments.

@alicederyn
Copy link
Contributor Author

Just to note I'm still addressing the feedback, just tackled the easier stuff first!

Copy link
Member
@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

Standard reminder: You can directly apply all the suggestions you want in one go with Files changed -> Add to batch -> Commit

Looking good so far! Just a couple quick suggestion followups to the existing fixes, aside from the bigger pending items. Thanks!

@CAM-Gerlach
Copy link
Member
CAM-Gerlach commented Feb 9, 2023

Just to note I'm still addressing the feedback, just tackled the easier stuff first!

Thanks @alicederyn ! I've resolved the implemented suggestions and checked the corresponding boxes in the checklist above.

Just FYI, to help make the easy stuff even easier, in case you aren't aware—instead of recreating the suggestions manually, you can apply them all directly in one go by going to the Files changed tab, clicking Add to batch on the suggestions you want, and then clicking Commit once you've selected all of them. This also auto-resolves the linked review comments, so they don't need to be double-checked and resolved manually.

@alicederyn
Copy link
Contributor Author

Just FYI, to help make the easy stuff even easier, in case you aren't aware—instead of recreating the suggestions manually, you can apply them all directly in one go

Thanks! I was aware of this, but I wanted to check everything rendered correctly locally and iterate, so I did it in the form of a PR to my PR branch. Sorry for creating extra work!

Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
@alicederyn
Copy link
Contributor Author

For reference, I have a bit of a rewrite at alicederyn#4 to address @JelleZijlstra's comments. I'm getting some internal feedback on it before I merge here, but happy for anyone to comment there first.

Define what TypedMapping does support, not just what it doesn't, to clarify edge cases like ``|``.
@CAM-Gerlach CAM-Gerlach dismissed their stale review February 14, 2023 06:22

All blocking changes resolved, thanks!

Copy link
Member
@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

LGTM from me, aside from still missing a number of recommended sections (see the checklist above) that would likely be quite helpful to include, even if just briefly for each. But that's ultimately up to you, @JelleZijlstra and @pablogsal .

@alicederyn
Copy link
Contributor Author

Backcompat and reference implementation sections at alicederyn#5 for internal review

Copy link
Member
@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

Thanks! Just "Security Implications" (which can be a very brief explanation of why there are none, or mentioning how this might improve user security) and "How to Teach This" suggested sections left, assuming there isn't a reason to omit them. Plus, a couple small suggestions/comments.

alicederyn and others added 2 commits February 21, 2023 10:06
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
alicederyn and others added 2 commits March 10, 2023 15:34
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
@alicederyn
Copy link
Contributor Author

@CAM-Gerlach I added a "How to teach" section. Pablo suggested leaving out the "security implications" section as it would be empty.

Copy link
Member
@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

A couple last comments/suggestions from my side; otherwise this LGTM!

Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Copy link
Member
@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

Thanks for all your hard work on this, @alicederyn ! @JelleZijlstra might have some last comments, but as far as I'm concerned, LGTM!

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.

I have a few more comments but those could also be addressed later. If you would prefer to have the PEP merged now, let me know and I'll do it.

* Remove `typing_extensions` implementation details; this is more than is necessary for the PEP.
* Do not mandate adding the PEP to the `typing` module docs, in case that practice ends before the PEP lands. Left in as a suggestion to mirror PEP 692.
@alicederyn alicederyn requested review from JelleZijlstra and hugovk and removed request for hugovk and JelleZijlstra March 13, 2023 08:54
@alicederyn
Copy link
Contributor Author

"Request rereview" has apparently changed behaviour and I have no idea how it now works :/ Apologies if I spammed anyone.

@CAM-Gerlach
Copy link
Member

"Request rereview" has apparently changed behaviour and I have no idea how it now works :/ Apologies if I spammed anyone.

Would be curious what specifically you're referring to—I've used this feature routinely over the past several years and haven't noticed any significant UI/UX changes, including just now on this PEP. Though, perhaps it had something to do with the fact that we both triggered it right around the same time, with me having loaded the page prior to you pressing the button?

@alicederyn
Copy link
Contributor Author

alicederyn requested review from JelleZijlstra and hugovk and removed request for hugovk and JelleZijlstra

I hit the request review icon for hugovk and Jelle as usual, but the end state was as if I hadn't, and this is the description of what I apparently did. Maybe it work differently if you have different privileges, and I've just never seen it before because I normally do this on repos where I have edit privileges?

@alicederyn
Copy link
Contributor Author

I've definitely seen this behaviour change a week ago in a private GH server, and those usually lag public GH changes by about a half year.

@CAM-Gerlach
Copy link
Member

Hmm, strange. I've put in a request about it to the others and GitHub.

@JelleZijlstra JelleZijlstra merged commit f749a27 into python:main Mar 14, 2023
@CAM-Gerlach
Copy link
Member

Congratulations @alicederyn , your PEP is now live at https://peps.python.org/pep-0705/ ! Now's the time to open a PEP discussion thread on Typing-SIG and/or the PEP category of the Python Discourse, and link it in the Discusions-To and the Post-History here. Thanks!

@alicederyn alicederyn deleted the pep.typedmapping branch March 14, 2023 13:21
@alicederyn
Copy link
Contributor Author

Thank you @CAM-Gerlach and @JelleZijlstra for your time and feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-pep A new draft PEP submitted for initial review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants
0