-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Open
gaogaotiantian
wants to merge
43
commits into
python:main
Choose a base branch
from
gaogaotiantian:pep-667-exec-impact
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 2 commits
Commits
Show all changes
43 commits
Select commit
Hold shift + click to select a range
ab71e9a
Add impact on exec() for PEP 667
gaogaotiantian 3d86025
Fix lint
gaogaotiantian 8729874
Apply suggestions from code review
gaogaotiantian e2f0b94
Apply @carljm suggestions from review
willingc 98edffa
Incorporate feedback and PyEval_GetLocals PR
ncoghlan 9a2a7d3
Syntax fixes, PEP 558 comparison tweaks
ncoghlan e67a37d
Move f_locals spec note to spec section
ncoghlan a2b6b33
Add missing space
ncoghlan 6be7b28
Indent ctypes example
ncoghlan f2423f0
Minor syntax and wording edits
ncoghlan 82f94fb
Reword mention of "real" locals.
ncoghlan 4a85e39
PEP link fix, minor text edits
ncoghlan 6115fbd
Add note about the extra PEP 558 C APIs being omitted
ncoghlan 669a0bc
Another link syntax fix
ncoghlan c68d02c
Merge branch 'main' into pep-667-exec-impact
ncoghlan 9a966dd
Fix incorrect prefix in function name
ncoghlan 98eddaf
Improve locals() implementation note
ncoghlan a8a700d
Clarify the note about deoptimization
ncoghlan 7808759
Be clear there is no `is_function` frame method
ncoghlan 6a89a65
More syntax and wording tweaks
ncoghlan 22494e4
Further updates
ncoghlan 828daa2
No joy on mixed syntax highlighting
ncoghlan 1bc2eaa
Remove leftover comma
ncoghlan a924f4b
Reword note about better optimization support
ncoghlan c5d35e1
Merge branch 'main' into pep-667-exec-impact
ncoghlan 32fc5fb
Address review comments from Barry & Tian
ncoghlan 6449585
Syntax fix
ncoghlan a0881b1
Fix typo
ncoghlan 8dec696
f_locals is mutable, not read-only!
ncoghlan 4151acb
Misc proofreading edits
ncoghlan 4871a39
Define exec/eval, fix proxy mutability
ncoghlan ad391ea
Fix syntax, update implementation note
ncoghlan 2269f65
Another syntax fix
ncoghlan 1539941
Make snapshot code consistent
ncoghlan 0c5aea9
Use 3.13 signature for eval spec
ncoghlan a846667
Merge branch 'main' into pep-667-exec-impact
ncoghlan 8abca04
Delay `PyEval_GetLocals` hard deprecation to 3.14
ncoghlan d3bff7b
Apply suggestions from code review
ncoghlan 40e59cb
Merge branch 'main' into pep-667-exec-impact
ncoghlan 1d4447d
Move locals rationale details to PEP 558
ncoghlan 80e115b
Merge remote-tracking branch 'origin/main' into pep-667-exec-impact
ncoghlan c84fc0d
Merge remote-tracking branch 'origin/main' into pep-667-exec-impact
ncoghlan b7b427f
Merge branch 'main' into pep-667-exec-impact
ncoghlan File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
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.
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.
Thanks for the first pass at trying to document this behavior. I would suggest reframing this a bit to:
locals()
to give the reader contextexec()
with performance optimizedlocals().
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.
For the semantic changes to
locals()
, it's mentioned in this PEP a bit earlier. We should assume the reader already reads that part right? How should I either refer back or summarize that?The reason of the behavior change is basically
About the expected behavior, do you mean something like "the local variable update will not be propagated between
exec()
"? I feel difficult to describe the exact behavior otherwise I would probably just summarize it in words instead of examples. This is a rather subtle change and the original behavior is kind of odd. I would love any help from people who are better at documentation.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.
Thanks @gaogaotiantian. We can definitely iterate on this. Looking at @carljm's message, I think there are three concepts that the reader may confuse:
locals()
,locals
argument, andframe.f_locals
for functions such asexec()
.I'm not sure what the best wording would be but I think we should iterate until it seems clear to folks. Mostly, I want to make sure that someone reading the PEP and using exec() has a clear idea of what the expected behavior should be.
P.S. I'm very happy that you are taking the time to make this PEP addition.
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.
@willingc I feel that these concepts are pretty well communicated by the current draft, with my suggested changes incorporated. First the current behavior of
exec()
is explained (with an example), then the reason for that behavior (and the connection tolocals()
is explained, then some additional confusing implications of the current behavior are shown (with an example), and then the new behavior is explained (which doesn't take very long, since it's much simpler), and then examples are given to demonstrate how to accomplish some things that are currently possible, under the new behavior. Are there particular aspects of this presentation that you don't find sufficiently clear?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.
Thanks @carljm. Let's accept your suggestions, and I will give it another read. I'm primarily concerned that the expected behavior for 3.13+ is clear to readers. That may be so after your changes are included. I have a hard time following the wording flow until I see it as one prose document.
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.
After my Discord comments about even PEP 558 not explaining the
locals()
changes particularly well, I went back to PEP 558, the related python-dev thread, and even PEP 3100 to find the material that actually went into the decision to makelocals()
andexec()
work as PEP 558 and PEP 667 both propose.The early iterations of PEP 558 actually tried to standardise
locals()
without changing it, so hopefully the new text makes it clear why that approach didn't last long :)