GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — PEP 667: Clarify specification ambiguities and the expected impact by gaogaotiantian · Pull Request #3845 · python/peps

Conversation

@gaogaotiantian

@gaogaotiantian gaogaotiantian commented Jun 19, 2024 •
edited by ncoghlan
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member
  • Change is:
    • To an Accepted or Final PEP, with Steering Council approval
    • To fix editorial issues (mostly missing context for changes originally discussed under PEP 558)
  • PR title prefixed with PEP number (e.g. PEP 123: Summary of changes)

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@gaogaotiantian
Add impact on exec() for PEP 667
ab71e9a
@gaogaotiantian
gaogaotiantian requested a review from markshannon as a code owner June 19, 2024 01:30
@gaogaotiantian
gaogaotiantian requested review from carljm and ncoghlan June 19, 2024 01:30
@gaogaotiantian
Fix lint
3d86025
carljm
carljm reviewed Jun 19, 2024
View reviewed changes

@carljm carljm left a comment

Copy link
Copy Markdown
Member

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 really good!

I think it might also make sense to add a "Rejected Alternatives" section, with a sub-section "Maintaining existing behavior of exec", discussing some of what we talked about in Discourse about the problems with trying to do this.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread peps/pep-0667.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread peps/pep-0667.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread peps/pep-0667.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread peps/pep-0667.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread peps/pep-0667.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@ncoghlan

ncoghlan commented Jun 19, 2024

Copy link
Copy Markdown
Contributor

The new section looks good to me. FWIW, What's New in 3.13 does discuss this aspect of the change (https://docs.python.org/dev/whatsnew/3.13.html#defined-mutation-semantics-for-locals), but having more details in the PEP definitely won't hurt.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ncoghlan

ncoghlan commented Jun 19, 2024

Copy link
Copy Markdown
Contributor

It may also be worth referencing back to the PEP 558 discussions of the impact of changing the way locals() works:

  • https://peps.python.org/pep-0558/#changing-locals-to-return-independent-snapshots-at-function-scope
  • https://peps.python.org/pep-0558/#keeping-locals-as-a-snapshot-at-function-scope

And while PEP 667's text didn't explicitly mention the impact on exec() and eval(), PEP 558 did: https://peps.python.org/pep-0558/#what-happens-with-the-default-args-for-eval-and-exec

This is a case where the use of competing PEPs may have let us down a bit - PEP 667 (quite reasonably) didn't bother repeating the rationale for the aspects where it agreed with the design decisions in PEP 558, so reading it in isolation from PEP 558 doesn't quite give the full picture of the change.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ncoghlan ncoghlan mentioned this pull request Jun 19, 2024
PEP 667: correct/clarify handling of PyEval_GetLocals python/steering-council#245
Closed
markshannon
markshannon reviewed Jun 20, 2024
View reviewed changes

@markshannon markshannon left a comment

Copy link
Copy Markdown
Member

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.

Looks good.
I've nothing to add beyond what @carljm and @ncoghlan say.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@gaogaotiantian

gaogaotiantian commented Jun 20, 2024

Copy link
Copy Markdown
Member Author

So should I post this on disclosure first?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

willingc
willingc reviewed Jun 20, 2024
View reviewed changes
Comment thread peps/pep-0667.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@carljm

carljm commented Jun 20, 2024

Copy link
Copy Markdown
Member

So should I post this on disclosure first?

Yes, this should be posted on Discourse to get community feedback, but it probably makes sense to address the existing feedback first?

willingc reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gaogaotiantian @carljm
Apply suggestions from code review
8729874
Co-authored-by: Carl Meyer <[email protected]>
@gaogaotiantian

gaogaotiantian commented Jun 21, 2024

Copy link
Copy Markdown
Member Author

Yes, this should be posted on Discourse to get community feedback, but it probably makes sense to address the existing feedback first?

Of course, just want to make sure about the procedure. I'm a bit swamped recently.

carljm reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ncoghlan

ncoghlan commented Jun 21, 2024

Copy link
Copy Markdown
Contributor

I ticked the SC approval box in the template, since we have that in python/steering-council#245 (comment)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@carljm

carljm commented Jun 21, 2024

Copy link
Copy Markdown
Member

since we have that

I could be misunderstanding, but I don't see an indication in that comment that the SC considered the issue of exec() at all. I thought that comment was about C APIs. My understanding is that we don't yet have SC approval (or even consideration) of the issues discussed in this PR.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

carljm
carljm reviewed Jun 21, 2024
View reviewed changes
Comment thread peps/pep-0667.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

carljm
carljm reviewed Jun 21, 2024
View reviewed changes
Comment thread peps/pep-0667.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread peps/pep-0667.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@willingc @carljm
Apply @carljm suggestions from review
e2f0b94
Co-authored-by: Carl Meyer <[email protected]>
@willingc

willingc commented Jun 21, 2024

Copy link
Copy Markdown
Contributor

@gaogaotiantian I accepted @carljm's suggestions so that I could look at the prose as it would display to a reader. I am going to add a few suggestions re: grammar, and then I believe this will be acceptable from my viewpoint. Thanks so much for doing this. It adds important clarity for all. 💐

ncoghlan reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

willingc
willingc reviewed Jun 21, 2024
View reviewed changes
Comment thread peps/pep-0667.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

willingc
willingc reviewed Jun 21, 2024
View reviewed changes
Comment thread peps/pep-0667.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

willingc
willingc reviewed Jun 21, 2024
View reviewed changes
Comment thread peps/pep-0667.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

willingc
willingc reviewed Jun 21, 2024
View reviewed changes
Comment thread peps/pep-0667.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

willingc
willingc reviewed Jun 21, 2024
View reviewed changes
Comment thread peps/pep-0667.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

willingc
willingc reviewed Jun 21, 2024
View reviewed changes
Comment thread peps/pep-0667.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

willingc
willingc reviewed Jun 21, 2024
View reviewed changes
Comment thread peps/pep-0667.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

willingc
willingc reviewed Jun 21, 2024
View reviewed changes
Comment thread peps/pep-0667.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

willingc
willingc reviewed Jun 21, 2024
View reviewed changes
Comment thread peps/pep-0667.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

ncoghlan
ncoghlan reviewed Sep 4, 2024
View reviewed changes
Comment thread peps/pep-0667.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Alyssa Coghlan
Merge branch 'main' into pep-667-exec-impact
544de0f
willingc
willingc approved these changes Sep 10, 2024
View reviewed changes
Yhg1s
Yhg1s approved these changes Oct 4, 2024
View reviewed changes
@ncoghlan ncoghlan mentioned this pull request Oct 9, 2024
PEP 667 follow-up: hard deprecate PyEval_GetLocals() python/cpython#125170
Open
markshannon
markshannon reviewed Oct 18, 2024
View reviewed changes

ghost left a comment

Copy link
Copy Markdown
Member

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.

Apologies for not getting to this sooner.

A few suggestions for moving stuff around a bit and a couple of very minor corrections.
Otherwise this looks good.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread peps/pep-0667.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread peps/pep-0667.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread peps/pep-0667.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread peps/pep-0667.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread peps/pep-0667.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread peps/pep-0667.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread peps/pep-0667.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Alyssa Coghlan Mark Shannon
Apply simple suggestions from code review
ac5c974
Co-authored-by: Mark Shannon <[email protected]>
ncoghlan
ncoghlan reviewed Oct 19, 2024
View reviewed changes

ghost left a comment •
edited by ncoghlan
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.

Mark's comments all make sense to me, so I'm working on an update now to address the ones which aren't simple line edits (I already accepted the simple changes).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread peps/pep-0667.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread peps/pep-0667.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread peps/pep-0667.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread peps/pep-0667.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread peps/pep-0667.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Alyssa Coghlan added 5 commits October 19, 2024 14:38
Alyssa Coghlan
Merge remote-tracking branch 'origin/main' into pep-667-exec-impact
e21a718
Alyssa Coghlan
Address initial set of recent review comments
709c345
* Move "Implementation Notes" after implementation section
* Avoid jargon when describing the deprecation notice plan
* Add inline comments to PyEval_GetLocals() replacement code

Additional changes:

* Add 2024 post-history entry
* Move PEP 709 impact to Backwards Compatibility section
Alyssa Coghlan
Extra keys can be removed
db15bf0
Alyssa Coghlan
Split 'Summary of Changes' out from 'Specification'
ea92f36
Alyssa Coghlan
Avoid duplicate heading name
9d812fd
ncoghlan
ncoghlan reviewed Oct 19, 2024
View reviewed changes

ghost left a comment

Copy link
Copy Markdown
Contributor

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.

Updates pushed to address most of @markshannon's feedback, but after some experimentation at the REPL, I still need to make some more changes to the paragraph about deleting values via frame proxy instances.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread peps/pep-0667.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Alyssa Coghlan added 3 commits October 19, 2024 17:33
Alyssa Coghlan
Extra key removal is implementation dependent
56727f0
Alyssa Coghlan
Add refs for del/pop regression
a27332d
Alyssa Coghlan
Fix typo
713ec6f
This was referenced Oct 19, 2024
gh-125590: Allow FrameLocalsProxy to delete and pop keys from extra locals python/cpython#125616
Merged
Document FrameLocalsProxy behaviour quirks python/cpython#125731
Open
Alyssa Coghlan
Merge branch 'main' into pep-667-exec-impact
f875410
markshannon
markshannon approved these changes Oct 21, 2024
View reviewed changes

ghost left a comment

Copy link
Copy Markdown
Member

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 really good now. Thanks.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ncoghlan reacted with hooray emoji
All reactions
  • 🎉 1 reaction
@ncoghlan
ncoghlan merged commit 07522bb into python:main Oct 21, 2024
@ncoghlan

ghost commented Oct 21, 2024

Copy link
Copy Markdown
Contributor

It was a journey, but we got there in the end :)

Thank you all. I have to share this bit from the generated squash merge showing how much of a group effort it really was:

Co-authored-by: Carl Meyer <...>
Co-authored-by: Carol Willing <...>
Co-authored-by: Alyssa Coghlan <...>
Co-authored-by: Petr Viktorin <...>
Co-authored-by: Mark Shannon <...>

(and of course @gaogaotiantian as the original PR author)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gaogaotiantian

ghost commented Oct 21, 2024

Copy link
Copy Markdown
Member Author

Thank you @ncoghlan for taking this over. I wouldn't be able to finish it like this :)

ncoghlan reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gaogaotiantian
gaogaotiantian deleted the pep-667-exec-impact branch October 21, 2024 17:53
Vako111
Vako111 reviewed Oct 22, 2024
View reviewed changes

ghost left a comment

Copy link
Copy Markdown

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.

Good

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Vako111
Vako111 reviewed Oct 22, 2024
View reviewed changes

ghost left a comment

Copy link
Copy Markdown

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.

He

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carljm carljm carljm left review comments
@gpshead gpshead gpshead left review comments
@warsaw warsaw warsaw approved these changes
@encukou encukou encukou left review comments
@ncoghlan ncoghlan ncoghlan left review comments
@willingc willingc willingc approved these changes
@Yhg1s Yhg1s Yhg1s approved these changes
@markshannon markshannon markshannon approved these changes
+1 more reviewer
@Vako111 Vako111 Vako111 left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

10 participants

@gaogaotiantian @ncoghlan @carljm @willingc @markshannon @gpshead @warsaw @encukou @Yhg1s @Vako111
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.