FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

PEP 649: Improve description of semantics and mark as accepted by larryhastings · Pull Request #3138 · python/peps · GitHub

/ peps Public

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

Merged
larryhastings merged 19 commits into
python:mainfrom
larryhastings:nail_down_annotation_expression_semantics
May 16, 2023
ConversationCommits (19)ChecksFiles changed
Merged

PEP 649: Improve description of semantics and mark as accepted#3138
larryhastings merged 19 commits into
python:mainfrom
larryhastings:nail_down_annotation_expression_semantics

Conversation

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

Copy link
Copy Markdown
Contributor
  • 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/

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 reviewed May 5, 2023
View reviewed changes
Comment thread pep-0649.rst Outdated
AA-Turner changed the title Improve PEP 649's description of its semantics. PEP 649: Improve description of semantics May 5, 2023
JelleZijlstra reviewed May 5, 2023
View reviewed changes
Comment thread pep-0649.rst Outdated
carljm reviewed May 5, 2023
View reviewed changes
Comment thread pep-0649.rst Outdated
AA-Turner mentioned this pull request May 5, 2023
Closed
AA-Turner reviewed May 5, 2023
View reviewed changes
Comment thread pep-0649.rst Outdated
Comment thread pep-0649.rst Outdated

JelleZijlstra commented May 8, 2023

Copy link
Copy Markdown
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 commented May 8, 2023

Copy link
Copy Markdown
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 commented May 8, 2023

Copy link
Copy Markdown
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
PEP 693: Postpone 3.12.0b1 by two weeks (python#3139)
9ace095
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>
Incorporate changes from feedback, mark Accepted.
8989c52

larryhastings commented May 8, 2023

Copy link
Copy Markdown
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 commented May 8, 2023

Copy link
Copy Markdown
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 commented May 9, 2023

Copy link
Copy Markdown
Contributor Author

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

larryhastings commented May 9, 2023

Copy link
Copy Markdown
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 reviewed May 9, 2023
View reviewed changes
Comment thread pep-0649.rst Outdated

AA-Turner commented May 9, 2023

Copy link
Copy Markdown
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 approved these changes May 9, 2023
View reviewed changes

carljm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread pep-0649.rst Outdated
Comment thread pep-0649.rst Outdated
Comment thread pep-0649.rst Outdated
Comment thread pep-0649.rst
Comment thread pep-0649.rst Outdated

larryhastings commented May 10, 2023

Copy link
Copy Markdown
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 reviewed May 10, 2023
View reviewed changes
Comment thread pep-0649.rst Outdated

larryhastings commented May 10, 2023

Copy link
Copy Markdown
Contributor Author

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

larryhastings commented May 10, 2023

Copy link
Copy Markdown
Contributor Author

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

Wow! That didn't last long.

JelleZijlstra approved these changes May 10, 2023
View reviewed changes

JelleZijlstra left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented May 10, 2023

Copy link
Copy Markdown
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 approved these changes May 10, 2023
View reviewed changes

carljm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Changes look good to me!

larryhastings mentioned this pull request May 10, 2023
Closed
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 reviewed May 10, 2023
View reviewed changes

CAM-Gerlach left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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]

Comment thread pep-0649.rst
Comment thread pep-0649.rst Outdated
CAM-Gerlach reviewed May 10, 2023
View reviewed changes
Comment thread pep-0649.rst
emilyemorehouse reviewed May 15, 2023
View reviewed changes
Comment thread pep-0649.rst Outdated

larryhastings commented May 15, 2023

Copy link
Copy Markdown
Contributor Author

The SC is happy with the changes.

python/steering-council#187

larryhastings and others added 3 commits May 15, 2023 16:02
Update post history with all conversations, courtesy CAM!
7da747a 
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Fix typo. Thanks, Emily!
c9b0773 
Co-authored-by: Emily Morehouse <emilyemorehouse@gmail.com>
Add "Discussions-To" header. Thanks, CAM!
c36b491 
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
larryhastings merged commit a111ec4 into python:main May 16, 2023
larryhastings deleted the nail_down_annotation_expression_semantics branch May 16, 2023 04:08
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

hugovk hugovk left review comments
emilyemorehouse emilyemorehouse left review comments
CAM-Gerlach CAM-Gerlach left review comments
carljm carljm approved these changes
JelleZijlstra JelleZijlstra approved these changes
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

Back | FazBrowse Home | New Git URL