gh-125590: Allow FrameLocalsProxy to delete and pop keys from extra locals#125616
Conversation
|
@terryjreedy the only tests failed were Idle on MacOS-13, which is a bit weird. I can imagine changing |
Sorry, something went wrong.
|
MacOS versions after 10 have various odd interactions with tkinter and python. Without a saved reference to the now-overwritten failing test, I cannot comment other than to note the following: The first call must result in the second, which I have no memory of. There is no direct test of either function. I'm glad you found better code that worked on major systems |
Sorry, something went wrong.
|
I think this needs backporting to 3.13, as that's the version mentioned in the issue |
Sorry, something went wrong.
markshannon
left a comment
There was a problem hiding this comment.
The exception types need changing from RuntimeError to KeyError.
Otherwise, looks good.
Sorry, something went wrong.
|
When you're done making the requested changes, leave the comment: |
Sorry, something went wrong.
|
FTR, from this PR, the potential additions to PEP 667 would be: I think this is what we want. k = KeyError(name)
k.add_note("Cannot delete local variable") |
Sorry, something went wrong.
|
From the documentation, it explains
I think there's a distinctive difference here. The key is in the mapping, we can even read it, |
Sorry, something went wrong.
|
I appreciate I guess the question is do we want: try:
del f.f_locals["local_name"]
except KeyError:
...to handle the exception or not? If not, then I think
Other than that, the PR looks ready to merge. |
Sorry, something went wrong.
I think we need two separate exceptions because I don't know what they would do in the
|
Sorry, something went wrong.
|
I also agree For python/peps#3845, I've described this with prose rather than the equivalent Python code:
(and then linked back to this PR and the associated bug report from the "Implementation Notes" section) For the main docs, looking at https://docs.python.org/dev/reference/datamodel.html#frame-objects there's nothing that currently goes into that level of detail. I've filed a separate docs issue (#125731) suggesting adding a 4th subsection there that talks more about the write-through proxy behaviour. |
Sorry, something went wrong.
|
Ok, let's go with ValueError. |
Sorry, something went wrong.
markshannon
left a comment
There was a problem hiding this comment.
Thanks
Sorry, something went wrong.
|
Thanks @gaogaotiantian for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13. |
Sorry, something went wrong.
|
Thanks @gaogaotiantian for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13. |
Sorry, something went wrong.
…xtra locals (pythonGH-125616) (cherry picked from commit 5b7a872) Co-authored-by: Tian Gao <gaogaotiantian@hotmail.com>
It's okay for us to pop/delete the keys that were added to
f_localsmanually, just ban the real fast variables.@ncoghlan not sure if there's any documentation that needs to be changed.
There are two types of exceptions that could be raised:
RuntimeErrorwhen users trying to remove a local variableKeyErrorwhen users trying to remove a key that does not exist