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 667: Clarify specification ambiguities and the expected impact #3845

Merged
merged 54 commits into from
Oct 21, 2024

Conversation

gaogaotiantian
Copy link
Member

@gaogaotiantian gaogaotiantian commented Jun 19, 2024

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

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.

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.

peps/pep-0667.rst Outdated Show resolved Hide resolved
peps/pep-0667.rst Outdated Show resolved Hide resolved
peps/pep-0667.rst Outdated Show resolved Hide resolved
peps/pep-0667.rst Outdated Show resolved Hide resolved
peps/pep-0667.rst Outdated Show resolved Hide resolved
@ncoghlan
Copy link
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.

@ncoghlan
Copy link
Contributor

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

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.

Copy link
Member

@markshannon markshannon 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.
I've nothing to add beyond what @carljm and @ncoghlan say.

@gaogaotiantian
Copy link
Member Author

So should I post this on disclosure first?

peps/pep-0667.rst Outdated Show resolved Hide resolved
@carljm
Copy link
Member

carljm commented Jun 20, 2024

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?

Co-authored-by: Carl Meyer <carl@oddbird.net>
@gaogaotiantian
Copy link
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.

@ncoghlan
Copy link
Contributor

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

@carljm
Copy link
Member

carljm commented Jun 21, 2024

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.

peps/pep-0667.rst Outdated Show resolved Hide resolved
peps/pep-0667.rst Outdated Show resolved Hide resolved
peps/pep-0667.rst Outdated Show resolved Hide resolved
Co-authored-by: Carl Meyer <carl@oddbird.net>
@willingc
Copy link
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. 💐

peps/pep-0667.rst Outdated Show resolved Hide resolved
peps/pep-0667.rst Outdated Show resolved Hide resolved
peps/pep-0667.rst Outdated Show resolved Hide resolved
peps/pep-0667.rst Outdated Show resolved Hide resolved
peps/pep-0667.rst Outdated Show resolved Hide resolved
peps/pep-0667.rst Outdated Show resolved Hide resolved
peps/pep-0667.rst Outdated Show resolved Hide resolved
peps/pep-0667.rst Outdated Show resolved Hide resolved
peps/pep-0667.rst Outdated Show resolved Hide resolved
@ncoghlan ncoghlan mentioned this pull request Sep 4, 2024
6 tasks
Copy link
Member

@markshannon markshannon left a 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.

peps/pep-0667.rst Outdated Show resolved Hide resolved
peps/pep-0667.rst Outdated Show resolved Hide resolved
peps/pep-0667.rst Outdated Show resolved Hide resolved
peps/pep-0667.rst Outdated Show resolved Hide resolved
peps/pep-0667.rst Outdated Show resolved Hide resolved
peps/pep-0667.rst Outdated Show resolved Hide resolved
peps/pep-0667.rst Show resolved Hide resolved
Co-authored-by: Mark Shannon <mark@hotpy.org>
Copy link
Contributor

@ncoghlan ncoghlan left a 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).

peps/pep-0667.rst Outdated Show resolved Hide resolved
peps/pep-0667.rst Outdated Show resolved Hide resolved
peps/pep-0667.rst Outdated Show resolved Hide resolved
peps/pep-0667.rst Outdated Show resolved Hide resolved
peps/pep-0667.rst Show resolved Hide resolved
* 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
Copy link
Contributor

@ncoghlan ncoghlan left a 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.

peps/pep-0667.rst Outdated Show resolved Hide resolved
Copy link
Member

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

@ncoghlan ncoghlan merged commit 07522bb into python:main Oct 21, 2024
6 checks passed
@ncoghlan
Copy link
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)

@gaogaotiantian
Copy link
Member Author

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

@gaogaotiantian gaogaotiantian deleted the pep-667-exec-impact branch October 21, 2024 17:53
Copy link

@Vako111 Vako111 left a comment

Choose a reason for hiding this comment

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

Good

Copy link

@Vako111 Vako111 left a comment

Choose a reason for hiding this comment

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

He

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.

10 participants