bpo-43605: Improve the documentation to exec() and eval()#25039
Conversation
- Add links to the relevant section in Language Reference about dynamic execution's limitation with respect to namespaces. - For eval(), move some explanatory text into a "Note" box. - Make the first paragraph of eval's doc consistent about what the first argument accepts. - For exec(), remove the text about the "globals" optional argument having to be a dict but not an instance of a subclass of dict. This is no longer true -- the code calls PyDict_Check(), not PyDict_CheckExact(). - Put quotes around the ``__builtins__`` in the text: clarify that in the context it means a string key in the dict passed to eval/exec as the globals dict. Otherwise, since the identifier __builtins__ refers to a module, it can be confusing and misleading. - Reordering some paragraphs so that overall the structure is improved. - Re-wrap some long lines in RST source.
There was a problem hiding this comment.
Thanks for submitting this PR to improve Python docs! I think this looks good overall, though I have a few minor comments below.
Additionally, I wonder if this would've been better if it were split into multiple smaller PRs? I found this slightly long to review.
Sorry, something went wrong.
Fidget-Spinner
left a comment
There was a problem hiding this comment.
LGTM. I think this might need a news item (sometimes significant docs changes require them). Lets see what the core dev says when they review this.
Sorry, something went wrong.
|
There's something I don't quite feel is right in the current doc about
This is unclear, because elsewhere (in Language Reference > Data Model, it says that the two are only "approximately" analogous:
Indeed this is a "key difference": a This works: >>> def f():
... fcvar = 0
... def g():
... class C:
... a = fcvar
... g()
>>> f()This doesn't: >>> def h():
... hcvar = 0
... def henc():
... exec("a = hcvar", globals(), locals())
... henc()
>>> h()
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "<stdin>", line 5, in h
File "<stdin>", line 4, in henc
File "<string>", line 1, in <module>
NameError: name 'hcvar' is not definedThe ... def henc():
... hcvar
... exec("a = hcvar", globals(), locals())But this works for a totally different reason -- we've already dereferenced the cell variable in the So, what if we just... remove that sentence?
|
Sorry, something went wrong.
|
Also, the footnote on
Is this still true? (Edit: the footnote was introduced in 2009. It's no longer the same parser anymore...) >>> exec("print('foo')\r\nprint('bar')\r\n")
foo
bar |
Sorry, something went wrong.
|
Also I'd like to be more affirmative in the Note box on "modifying the default local namespace returned by |
Sorry, something went wrong.
The documentations for eval/exec are further modified for consistency and accuracy. - For both functions, the documentation mostly follows the following structure: description of arguments, default values for optional arguments, special treatment of the "__builtins__" key, and notices about limitations or other noteworthy material. Previously, the description about the special "__builtins__" key is intermingled with the description about default values for missing arguments. Now, these are in separate paragraphs. Also, it is stressed that the special key is inserted in-place. - In the notice about limitations to dynamic eval/exec w.r.t. scopes, also mention the assignment expression succinctly. - Remove the description that exec() with both globals and locals behave "as if" the source string is embedded in a class block. Again, a class block inside a function can access the non-locals, but this is not available to exec(). - Instead of mentioning passing the return values of globals() and locals() around, we now say that the programmer can construct their own namespaces based on their return values. The reason is that currently the semantics of locals() is not very well defined, and in the light of PEP 558 it may change in the future. Explicitly constructing new namespaces is more predictable than using the "raw" return value of locals(). In the future, this part of the docs can be further updated to keep up with the new definition of locals(). - Omit the footnote to exec() about the parser only accepting Unix-style LF newlines. This piece of information is outdated. - Other small fixes for grammar.
|
I've updated the PR with a new commit, in the hope that clarity/accuracy is further improved. If the review is in favour of the proposed revision, I'll add a NEWS entry. |
Sorry, something went wrong.
|
This PR is stale because it has been open for 30 days with no activity. |
Sorry, something went wrong.
|
This unfortunately has a couple of merge conflicts now. Would you mind fixing those? |
Sorry, something went wrong.
|
@congma Would you still be interested in fixing the merge conflicts? I can open a separate one to get the conflicts resolved if not. |
Sorry, something went wrong.
iritkatriel
left a comment
There was a problem hiding this comment.
This has merge conflicts now.
Sorry, something went wrong.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase And if you don't make the requested changes, you will be put in the comfy chair! |
Sorry, something went wrong.
This does have merge conflicts, so a new PR (#100003) was opened to resolve part of the conflict. I'll open another PR on |
Sorry, something went wrong.
|
The following commit authors need to sign the Contributor License Agreement: |
Sorry, something went wrong.
|
This PR is stale because it has been open for 30 days with no activity. |
Sorry, something went wrong.
|
Hello! I'm sorry but I'm closing this PR as it is stale, has requested changes that have not been addressed in quite some time, an unsigned CLA, and the issue as been resolved. |
Sorry, something went wrong.
execution's limitation with respect to namespaces.
argument accepts.
having to be a dict but not an instance of a subclass of dict. This is
no longer true -- the code calls PyDict_Check(), not
PyDict_CheckExact().
__builtins__in the text: clarify that inthe context it means a string key in the dict passed to eval/exec as
the globals dict. Otherwise, since the identifier builtins refers
to a module, it can be confusing and misleading.
https://bugs.python.org/issue43605