Skip to content

PEP 649: Improve description of semantics and mark as accepted #3138

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 19 commits into from
May 16, 2023
Merged

PEP 649: Improve description of semantics and mark as accepted #3138

merged 19 commits into from
May 16, 2023

Conversation

larryhastings
Copy link
Contributor

@larryhastings larryhastings commented May 5, 2023 •
edited by github-actions bot
Loading

  • Stipulate that name resolution for annotations under 649 must be identical to stock semantics.
  • Don't specify exactly the mechanism that conformant implementations must use to implement 649. Instead, do describe how CPython might do it, but leave the actual details of how to implement 649 up to each language implementation.
  • Incorporate Jelle's suggestion that the formats for inspect.get_annotations() be in an enum.IntEnum.

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

@larryhastings
Improve PEP 649's description of its semantics.
d9a17ff 
* Stipulate that name resolution for annotations
  under 649 must be *identical* to stock semantics.
* *Don't* specify exactly the mechanism that conformant
  implementations must use to implement 649.  Instead,
  *do* describe how CPython might do it, but leave the
  actual details of how to implement 649 up to each
  language implementation.
* Incorporate Jelle's suggestion that the formats for
  inspect.get_annotations() be in an enum.IntEnum.
hugovk
hugovk reviewed May 5, 2023
View reviewed changes
@AA-Turner AA-Turner changed the title Improve PEP 649's description of its semantics. PEP 649: Improve description of semantics May 5, 2023
JelleZijlstra
JelleZijlstra reviewed May 5, 2023
View reviewed changes
carljm
carljm reviewed May 5, 2023
View reviewed changes
@AA-Turner AA-Turner mentioned this pull request May 5, 2023
Closed
AA-Turner
AA-Turner reviewed May 5, 2023
View reviewed changes
@JelleZijlstra
Copy link
Member

A minor suggestion brought over from #3127: maybe we should drop FORMAT_MIN and FORMAT_MAX? I don't see much use for them if there is an enum; if you really want these values you can do min(member.value for member in inspect.AnnotationFormat).

@larryhastings
Copy link
Contributor Author

FWIW, I duplicated some work in this PR that was already in another PR, #3127. Having made the mistake, it seemed easiest to close the other PEP and do everything in this one PR. Sorry for the miasma.

@larryhastings
Copy link
Contributor Author

A minor suggestion brought over from #3127: maybe we should drop FORMAT_MIN and FORMAT_MAX?

Done. Iterating over the enum is a better solution.

hugovk and others added 3 commits May 8, 2023 15:24
@hugovk @larryhastings
PEP 693: Postpone 3.12.0b1 by two weeks (python#3139)
9ace095
@JelleZijlstra @AlexWaygood
@hauntsaninja @larryhastings
PEP 695: Lazy evaluation, concrete scoping semantics, other changes (p…
108f275 
…ython#3122)

- Lazy evaluation means that referencing a later type variable works at runtime
- Disallow walrus in TypeVar bounds, and also disallow yield/yield from/await
  in the same contexts
- Remove rejection of lambda lifting; that is the implementation we are using now
- Change the AST
- Change of direction on mangling
- More precise scoping rules

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-authored-by: Shantanu <12621235+hauntsaninja@users.noreply.github.com>
@larryhastings
Incorporate changes from feedback, mark Accepted.
8989c52
@larryhastings
Copy link
Contributor Author

Huh? GitHub decided to request Guido and Thomas as reviewers. I didn't ask for that. I've removed Guido and Thomas from review requests--obviously, they're welcome to review the PR, but I don't think it's necessary. Sorry for the churn, folks.

@JelleZijlstra
Copy link
Member

Huh? GitHub decided to request Guido and Thomas as reviewers. I didn't ask for that. I've removed Guido and Thomas from review requests--obviously, they're welcome to review the PR, but I don't think it's necessary. Sorry for the churn, folks.

I think you incorrectly merged in the two most recent commits on the main branch (which happen to be on 693 and 695). I'm going to click the "Update branch" button to get those commits out of the PR branch.

@larryhastings
Copy link
Contributor Author

Thanks, Jelle. Thank goodness Git is so conceptually simple and easy to use!

@larryhastings
Copy link
Contributor Author

@AA-Turner, on the other thread you said "does this list need updating?" with regards to the format IntEnums, and I didn't know what you meant. I just now noticed: in one section, the FORWARDREF enum is 2, and in another section it's 3. Is that what you were referring to?

(And, of course, I'm fixing that now. Time to pick the final values!)

hugovk
hugovk reviewed May 9, 2023
View reviewed changes
@AA-Turner
Copy link
Member

@AA-Turner, on the other thread you said "does this list need updating?" with regards to the format IntEnums, and I didn't know what you meant.

I was referring to the repr for VALUE &c -- you'd updated the list starting on line 576 to read inspect.AnnotationFormat.VALUE, but the list starting on line 78 still read inspect.VALUE. This PR consistently uses inspect.VALUE et al., so I think the comment is moot. Thanks for following up!

A

carljm
carljm approved these changes May 9, 2023
View reviewed changes
Copy link
Member

@carljm carljm left a comment

Choose a reason for hiding this comment

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

Left some additional comments, but I don't feel super strongly about any of them; generally this looks good to me!

@larryhastings
Copy link
Contributor Author

Somehow I'm unable to reply directly to Carl's most recent comment. He said:

We don't have to explicitly document support for everything that is technically possible. Are there strong use cases for someone using these public inspection methods to not use the enum values?

There are not, and I expect everyone to use the enum values. I was just trying to be precise.

But, we already defined the allowable values for __annotate__; why are we defining it twice? How about we simplify the text as follows:

The format parameter on these two functions accept the same values
as the format parameter on the __annotate__ magic method
defined above; however, these format parameters also have a default
value of inspect.VALUE.

JelleZijlstra
JelleZijlstra reviewed May 10, 2023
View reviewed changes
@larryhastings
Copy link
Contributor Author

Fingers crossed that we can ship 72b8183. I believe I've addressed everybody's comments (and basically incorporated everyone's suggestions).

@larryhastings
Copy link
Contributor Author

In the words of the rare "Big Bang Bar" pinball machine:

Wow! That didn't last long.

JelleZijlstra
JelleZijlstra approved these changes May 10, 2023
View reviewed changes
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.

All the changes look good to me, thanks!

Technically, changes to accepted PEPs need SC approval. I doubt the SC would have a big problem with anything in this PR, but ideally you'll have to get their signoff. Who knows, maybe they'll insist that we really need that __locals__ attribute on functions.

@larryhastings
Copy link
Contributor Author

Good call. I'll ping them once we're collectively happy with these changes. I bet we'll get a quick rubber stamp, and then I'll merge.

carljm
carljm approved these changes May 10, 2023
View reviewed changes
Copy link
Member

@carljm carljm left a comment

Choose a reason for hiding this comment

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

Changes look good to me!

@larryhastings larryhastings mentioned this pull request May 10, 2023
Closed
@CAM-Gerlach CAM-Gerlach changed the title PEP 649: Improve description of semantics PEP 649: Improve description of semantics and mark as accepted May 10, 2023
CAM-Gerlach
CAM-Gerlach reviewed May 10, 2023
View reviewed changes
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 by going to Files changed -> Clicking Add to batch on each suggestion -> When done, clicking Commit

It should be noted that with this PR, the PEP is marked as accepted—I've updated the title accordingly to note this key fact.

Accepted PEP checklist:

  • SC/PEP Delegate has formally accepted/rejected the PEP and posted to the Discussions-To thread
  • Pull request title in appropriate format (PEP 123: Mark as Accepted)
  • Status changed to Accepted/Rejected
  • Resolution link points directly to SC/PEP Delegate official acceptance/rejected post
  • Acceptance/rejection notice added, if the SC/PEP delegate had major conditions or comments N/A besides updating the Python-Version
  • Discussions-To, Post-History and Python-Version up to date [Done in suggestions]

emilyemorehouse
emilyemorehouse reviewed May 15, 2023
View reviewed changes
@larryhastings
Copy link
Contributor Author

The SC is happy with the changes.

python/steering-council#187

larryhastings and others added 3 commits May 15, 2023 16:02
@larryhastings @CAM-Gerlach
Update post history with all conversations, courtesy CAM!
7da747a 
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
@larryhastings @emilyemorehouse
Fix typo. Thanks, Emily!
c9b0773 
Co-authored-by: Emily Morehouse <emilyemorehouse@gmail.com>
@larryhastings @CAM-Gerlach
Add "Discussions-To" header. Thanks, CAM!
c36b491 
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
@larryhastings larryhastings merged commit a111ec4 into python:main May 16, 2023
@larryhastings larryhastings deleted the nail_down_annotation_expression_semantics branch May 16, 2023 04:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hugovk hugovk hugovk left review comments

@emilyemorehouse emilyemorehouse emilyemorehouse left review comments

@CAM-Gerlach CAM-Gerlach CAM-Gerlach left review comments

@carljm carljm carljm approved these changes

@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes

@AA-Turner AA-Turner AA-Turner approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@larryhastings @JelleZijlstra @AA-Turner @carljm @hugovk @emilyemorehouse @CAM-Gerlach