Skip to content

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
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

@larryhastingslarryhastings commented May 5, 2023

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/

* 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.
Comment threadpep-0649.rst Outdated
@AA-TurnerAA-Turner changed the title Improve PEP 649's description of its semantics.PEP 649: Improve description of semanticsMay 5, 2023
Comment threadpep-0649.rst Outdated
Comment threadpep-0649.rst Outdated
Comment threadpep-0649.rst Outdated
Comment threadpep-0649.rst Outdated
@JelleZijlstra

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

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
ContributorAuthor

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.

hugovkand others added 3 commits May 8, 2023 15:24
…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

Copy link
Copy Markdown
ContributorAuthor

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
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

Copy link
Copy Markdown
ContributorAuthor

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

@larryhastings

Copy link
Copy Markdown
ContributorAuthor

@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!)

Comment threadpep-0649.rst Outdated
@AA-Turner

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

@carljmcarljm 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.

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

Comment threadpep-0649.rst Outdated
Comment threadpep-0649.rst Outdated
Comment threadpep-0649.rst Outdated
Comment threadpep-0649.rst
Comment threadpep-0649.rst Outdated
@larryhastings

Copy link
Copy Markdown
ContributorAuthor

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.

Comment threadpep-0649.rst Outdated
@larryhastings

Copy link
Copy Markdown
ContributorAuthor

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

@larryhastings

Copy link
Copy Markdown
ContributorAuthor

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

Wow! That didn't last long.

@JelleZijlstraJelleZijlstra 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.

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
Copy Markdown
ContributorAuthor

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.

@carljmcarljm 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.

Changes look good to me!

@CAM-GerlachCAM-Gerlach changed the title PEP 649: Improve description of semanticsPEP 649: Improve description of semantics and mark as acceptedMay 10, 2023

@CAM-GerlachCAM-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.

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 threadpep-0649.rst
Comment threadpep-0649.rst Outdated
Comment threadpep-0649.rst
Comment threadpep-0649.rst Outdated
@larryhastings

Copy link
Copy Markdown
ContributorAuthor

The SC is happy with the changes.

python/steering-council#187

larryhastingsand others added 3 commits May 15, 2023 16:02
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Co-authored-by: Emily Morehouse <emilyemorehouse@gmail.com>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
@larryhastings
larryhastings merged commit a111ec4 into python:mainMay 16, 2023
@larryhastings
larryhastings deleted the nail_down_annotation_expression_semantics branch May 16, 2023 04:08
Sign up for freeto 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.

7 participants

@larryhastings@JelleZijlstra@AA-Turner@carljm@hugovk@emilyemorehouse@CAM-Gerlach