FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-46328: added sys.exception() by iritkatriel · Pull Request #30514 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

bpo-46328: added sys.exception() - #30514

Merged
iritkatriel merged 6 commits into
python:mainfrom
iritkatriel:bpo-46328-sys-exception
Jan 13, 2022
ConversationCommits (6)ChecksFiles changed
Merged

bpo-46328: added sys.exception()#30514
iritkatriel merged 6 commits into
python:mainfrom
iritkatriel:bpo-46328-sys-exception

Conversation

iritkatriel commented Jan 10, 2022 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

tiran reviewed Jan 10, 2022
View reviewed changes
Comment thread Doc/library/sys.rst Outdated
iritkatriel mentioned this pull request Jan 11, 2022
Merged
iritkatriel requested a review from warsaw January 12, 2022 17:34
vstinner reviewed Jan 12, 2022
View reviewed changes

vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Please document the new function in Doc/whatsnew/3.11.rst.

Comment thread Python/sysmodule.c Outdated
\n\
displayhook() -- print an object to the screen, and save it in builtins._\n\
excepthook() -- print an exception and its traceback to sys.stderr\n\
exception() -- return the current exception (thread safe)\n\

vstinner Jan 12, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

What is this "(thread safe)" note? It's not mentioned anywhere else.

iritkatriel Jan 12, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The next line for exc_info mentions it's thread-safe, so I didn't want it to look like this isn't. But maybe I should remove it from both lines?

vstinner Jan 13, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

My concern is more that it's only in help(sys), but nowhere else. If it's thread safe and this information matters, it should be in the documentation as well. Maybe also in the function docstring.

iritkatriel Jan 13, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I don’t know if it matters. This info is thread specific anyway, so you would assume this is thread safe (this is mentioned in the doc).

iritkatriel Jan 13, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Are the other functions in sys, like getrefcount, not thread safe?

iritkatriel Jan 13, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think it could be that the intention in the help line for exc_info was to say that it’s a thread-specific value, but it was worded incorrectly. I’d change both to say this instead of thread-safe.

vstinner Jan 13, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Most functions implemented in C are thread-safe. I agree, remove the mention from both functions.

The exact behavior can be elaborated in the documentation, if needed.

Yeah, the current exception is per-thread.

Comment thread Lib/test/test_sys.py
vstinner approved these changes Jan 13, 2022
View reviewed changes

vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM. But I suggest to remove "(thread safe)" from sys docstring.

iritkatriel merged commit c590b58 into python:main Jan 13, 2022

vstinner commented Jan 13, 2022

Copy link
Copy Markdown
Member

Nice @iritkatriel! That looks like an useful feature ;-) I dislike sys.exc_info() which smells like a legacy of Python 2 exceptions.

iritkatriel deleted the bpo-46328-sys-exception branch January 13, 2022 15:32
coderabbitai Bot mentioned this pull request Jul 17, 2026
Open
This file contains hidden or 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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

tiran tiran left review comments
vstinner vstinner approved these changes
warsaw Awaiting requested review from warsaw

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL