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 646: Add section on type substitution in generic aliases #2883

Merged
merged 4 commits into from Nov 27, 2022

Conversation

mrahtz
Copy link
Contributor

@mrahtz mrahtz commented Nov 12, 2022

As agreed in python/cpython#98267 (comment), and tentatively approved by the SC in python/steering-council#145, here's a retroactive amendment to PEP 646 detailing the rules for type substitution in generic aliases, as determined in python/cpython#91162 post-PEP-acceptance, and currently only documented in test_typing.py at https://github.com/python/cpython/blob/a0bc75e2fdd53680cb147881bcb3754bd56aa2fa/Lib/test/test_typing.py#L592.

This is a bit complicated, so happy to iterate for better pedagogy.

@gvanrossum @serhiy-storchaka @pradeep90

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. I reviewed the added text and aside from a probable typo in an example, a syntax change and one reST tip, I didn't find any textual or technical issues with the additions.

I'm not a subject matter expert, so that'll be up to @gvanrossum / @JelleZijlstra to review that aspect. Presumably, you'll want SC approval of the changes here before they are merged, via a SC issue, or at the very least a SC member approving this on their behalf. I see mentions of one on the first linked issue, but couldn't find any actual cross-references/links to the same; however, I'm guessing the referenced issue might be python/steering-council#145. You might want to mark this PR as Draft so it isn't accidentally merged before then.

pep-0646.rst Outdated Show resolved Hide resolved
pep-0646.rst Outdated Show resolved Hide resolved
pep-0646.rst Outdated Show resolved Hide resolved
pep-0646.rst Show resolved Hide resolved
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 except for CAM's suggestions.

mrahtz and others added 3 commits November 13, 2022 16:40
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
@mrahtz mrahtz marked this pull request as draft November 13, 2022 16:43
@mrahtz
Copy link
Contributor Author

mrahtz commented Nov 13, 2022

Thank you for all the suggestions CAM! I've also added the link to python/steering-council#145 to the root comment, and converted the PR to a draft so we can wait a while to see whether anyone else wants to chime in here.

Copy link
Member

@gvanrossum gvanrossum 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, very clear. I especially appreciated the final section ("TypeVarTuples Cannot be Split") which explained the one niggling question my brain started to formulate just as I arrived at that point. Well done!

I say mark it as "not draft", and ping the SC issue. Someone there should get back to you.

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 as well from an editorial perspective, pending SC review and approval.

@CAM-Gerlach
Copy link
Member

I say mark it as "not draft", and ping the SC issue.

Only issue with that is someone could easily see three PEP editor ✔️s and prematurely, not noticing the details in the comments that we're waiting for SC approval, as has happened before—using the Draft state makes it clear the PR is not yet ready to be merged, and prevents merging (without first explicitly marking it as ready to merge). There's also the do-not-merge label, but it relies on someone noticing it and otherwise has no actual effect. We could also have a special label like awaiting-sc-approval that blocks merging via a required status check, but that would make it look like the build itself was failing on the PR and individual commits unless digging into the detailed checks, which could be confusing and noisy.

@encukou
Copy link
Member

encukou commented Nov 15, 2022

Guido, I found this old quote from you and I'm interested if/how your views changed:

I strongly disagree with using (standards-track) PEPs as mutable
documents -- before you know it people will have to use weasel words
like "compliant with PEP 333 as of date X" to describe their
software's conformity. If you add a new PEP #, software declared
compliant with PEP 333 remains compliant (and some such software can
now also claim compliance with the new PEP without any changes).

I don't mean to be hostile -- I am genuinely interested in your current thoughts. (And I'm half expecting to hear you resigned so you wouldn't have to deal with process.)


I added this to the SC agenda.
I expect lots of discussion about the process. But besides that, the modifications only affect type hints, not syntax or other parts of Python. I'll recommend that the SC delegates the decision to typing-sig, represented by Guido & Jelle.

@gvanrossum
Copy link
Member

Guido, I found this old quote from you and I'm interested if/how your views changed:

I strongly disagree with using (standards-track) PEPs as mutable
documents -- before you know it people will have to use weasel words
like "compliant with PEP 333 as of date X" to describe their
software's conformity. If you add a new PEP #, software declared
compliant with PEP 333 remains compliant (and some such software can
now also claim compliance with the new PEP without any changes).

I don't mean to be hostile -- I am genuinely interested in your current thoughts. (And I'm half expecting to hear you resigned so you wouldn't have to deal with process.)

It is a difficult problem. As pertaining to this particular PEP and PR, I believe that the addition here is merely a clarification of behavior that we didn't specify sufficiently but thought would be obvious (it wasn't). The behavior being clarified here is already released as part of 3.11.0.

In general, I still think that PEPs are primarily change proposals and historical documents. Some time after a PEP has been finalized we may extend or even deprecate part of an API that was specified in the PEP, and we shouldn't update the PEP to reflect that (except perhaps in some cases by adding carefully worded notes pointing out that the PEP no longer describes the implementation in some future version -- but these are not normative, just hints for the reader). If the change is big enough, it requires a new PEP. Otherwise, it just requires discussion, an issue, a PR, and docs.

For "standards" that are important for a subcommunity (packaging, typing) that keep updated, maybe we should use something other than PEPs. Maybe we could have a new PEP category, similar to "Informational" (those can freely be updated), but binding for that subcommunity (important when there are multiple 3rd party implementations trying to interoperate).

I don't think we have to be as rigid as the IETF RFC process. (Where even the formatting can never be changed.)

But I don't think we should retroactively change the substance of old PEPs (fixing links or clarifying wording that was clearly confusing or mistaken is typically okay, but the SC should keep an eye on this).

What we did with PEP 484 (the original typing PEP) was a bit questionable, but that PEP was so monumental and under-specified that we felt the best option was still to update the PEP when we changed our mind about certain details. Also, that PEP had "provisional" status for a very long time. Big new typing ideas always got their own PEP anyways.

Hopefully that's clearer than mud.

I added this to the SC agenda. I expect lots of discussion about the process. But besides that, the modifications only affect type hints, not syntax or other parts of Python. I'll recommend that the SC delegates the decision to typing-sig, represented by Guido & Jelle.

Yeah, the SC is there for process. Good luck. Jelle and I have already approved this PR.

@encukou
Copy link
Member

encukou commented Nov 17, 2022

Thank you! It's pretty clear :)

Copy link
Member

@encukou encukou left a comment

Choose a reason for hiding this comment

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

No objections from the SC!

@gvanrossum gvanrossum marked this pull request as ready for review November 24, 2022 15:28
@gvanrossum
Copy link
Member

The SC approved this change. Anything you want to change before I land it?

@mrahtz
Copy link
Contributor Author

mrahtz commented Nov 24, 2022

Excellent! Go for it :)

@gvanrossum gvanrossum merged commit 2ae3b71 into python:main Nov 27, 2022
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

6 participants