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

gh-78612: Mark up eval() using param list #115212

Merged
merged 5 commits into from Feb 28, 2024

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Feb 9, 2024

Also mention that the *expression* parameter can be a string.
@erlend-aasland
Copy link
Contributor Author

Competing PR to #20000

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Feb 9, 2024

Screenshot of eval() param list Screenshot 2024-02-14 at 10 35 31

The arguments are a string and optional globals and locals. If provided,
*globals* must be a dictionary. If provided, *locals* can be any mapping
object.
:param expression:
Copy link
Member

Choose a reason for hiding this comment

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

This is currently the only function in the page to use :param:. Are you planning to update some/all the others as well?
If not, I think it might be better to keep the original paragraph. Mentioning that it accepts code objects and adding links to that and to "mapping" are good changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it matters that only one function is marked up better. In any case, there are other good candidates:

  • compile()
  • exec()
  • open()
  • print()
  • __import__()

Possibly also:

  • setattr()
  • sorted()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FTR, I'd suggest adapting one at the time.

@erlend-aasland

This comment was marked as resolved.

Comment on lines +541 to +542
:returns: The result of the evaluated expression.
:raises: Syntax errors are reported as exceptions.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These two sentences were copied verbatim from the original text. I don't think it makes sense to expand the scope of this PR to include tweaking the wording in these two sentences.

@erlend-aasland
Copy link
Contributor Author

@iritkatriel, are you interested in reviewing this?

Copy link
Contributor

@willingc willingc left a comment

Choose a reason for hiding this comment

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

Let's perhaps move this PR forward to merge. We can get folks' feedback at the next docs meeting on the formatting.

@erlend-aasland
Copy link
Contributor Author

Let's perhaps move this PR forward to merge. We can get folks' feedback at the next docs meeting on the formatting.

I'm fine with that. @CAM-Gerlach suggested broadcasting this change to a wider audience, and I think that's a good idea; I just don't have the available time to start (and continue to follow) such a discussion right now. If you're fine with trying this out and discussing it at the next docs meeting, I'm fine with that. (If we don't like it, we can revert 🙂)

Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

Looks clearer to me, easier to find info when scanning a webpage, rather than reading like a book.

Before After

@erlend-aasland
Copy link
Contributor Author

Thanks for chiming in, everyone. We'll land this now and discuss the change at the upcoming docs meetup (2024-03-05).

@erlend-aasland erlend-aasland merged commit a71e32c into python:main Feb 28, 2024
25 checks passed
@erlend-aasland erlend-aasland deleted the docs/eval branch February 28, 2024 13:03
@miss-islington-app
Copy link

Thanks @erlend-aasland for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 28, 2024
Also mention that the 'expression' parameter can be a string.
(cherry picked from commit a71e32c)

Co-authored-by: Erlend E. Aasland <erlend@python.org>
@bedevere-app
Copy link

bedevere-app bot commented Feb 28, 2024

GH-116044 is a backport of this pull request to the 3.12 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 28, 2024
Also mention that the 'expression' parameter can be a string.
(cherry picked from commit a71e32c)

Co-authored-by: Erlend E. Aasland <erlend@python.org>
@bedevere-app bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Feb 28, 2024
@bedevere-app
Copy link

bedevere-app bot commented Feb 28, 2024

GH-116045 is a backport of this pull request to the 3.11 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.11 only security fixes label Feb 28, 2024
@erlend-aasland
Copy link
Contributor Author

Let's hold the backports until after the docs meeting.

woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request Mar 4, 2024
Also mention that the 'expression' parameter can be a string.
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 25, 2024
Also mention that the 'expression' parameter can be a string.
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
Also mention that the 'expression' parameter can be a string.
erlend-aasland added a commit that referenced this pull request May 7, 2024
Also mention that the 'expression' parameter can be a string.
(cherry picked from commit a71e32c)

Co-authored-by: Erlend E. Aasland <erlend@python.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docs does not mention that eval() allows a code object as an argument
4 participants