-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
PEP 705: TypedMapping #2997
Conversation
I confirm I am sponsoring this PEP 👍 |
Please use PEP number 705. (Re-allocating from #2988 which was closed because it didn't yet find a sponsor.) |
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.
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>
) andPEP
header [resolved in suggestion] - Title clearly, accurately and concisely describes the content in 79 characters or less
-
Author
,Status
(Draft
),Type
andCreated
headers filled out correctly -
PEP-Delegate
,Topic
,Requires
andReplaces
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 suchN/A- Right before or after initial merging, PEP discussion thread created and linked to in
Discussions-To
andPost-History
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 quite good and makes a convincing case for the feature, but I have a few comments.
Just to note I'm still addressing the feedback, just tackled the easier stuff first! |
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.
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!
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 |
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>
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 ``|``.
All blocking changes resolved, thanks!
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 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 .
Backcompat and reference implementation sections at alicederyn#5 for internal review |
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.
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.
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com> Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
@CAM-Gerlach I added a "How to teach" section. Pablo suggested leaving out the "security implications" section as it would be empty. |
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 couple last comments/suggestions from my side; otherwise this LGTM!
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
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.
Thanks for all your hard work on this, @alicederyn ! @JelleZijlstra might have some last comments, but as far as I'm concerned, LGTM!
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.
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.
"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? |
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? |
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. |
Hmm, strange. I've put in a request about it to the others and GitHub. |
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 |
Thank you @CAM-Gerlach and @JelleZijlstra for your time and feedback! |
Proposal to add a TypedMapping protocol to the language.
Sponsor: @pablogsal