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

gh-82378 fix sys.tracebacklimit in pyrepl by cfbolz · Pull Request #123062 · python/cpython · GitHub

/ cpython Public

gh-82378 fix sys.tracebacklimit in pyrepl - #123062

Merged
pablogsal merged 10 commits into
python:mainfrom
cfbolz:gh-82378-fix-tracebacklimit-in-pyrepl-simplification
Aug 18, 2024
ConversationCommits (10)ChecksFiles changed
Merged

gh-82378 fix sys.tracebacklimit in pyrepl#123062
pablogsal merged 10 commits into
python:mainfrom
cfbolz:gh-82378-fix-tracebacklimit-in-pyrepl-simplification

Conversation

cfbolz commented Aug 16, 2024 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Contributor

This is a slightly more invasive approach to #82378 than the one in #122452. In addition to the changes there, the PR also changes the way that _pyrepl.console.InteractiveColoredConsole interacts with code.InteractiveConsole. The PR removes new keyword arguments colorize (and limit) of the methods code.InteractiveConsole.showsyntaxerror and .showtraceback. The keyword arguments were added only for pyrepl, and not documented and only indirectly tested. Instead of using these keyword arguments, pyrepl now overwrites the new _showtraceback helper method.

The new undocumented keyword arguments broke some third-party libraries on PyPy pypy/pypy#5004 (comment), however in ways that are extremely unlikely to affect CPython.

I cannot decide whether this is a too radical change this late in the release process. @serhiy-storchaka what do you think?

cfbolz added 5 commits July 30, 2024 14:01
pythongh-82378: fix sys.tracebacklimit in pyrepl
1d4f321 
make sure that pyrepl uses the same logic for sys.tracebacklimit as both
the basic repl and the standard sys.excepthook
Merge branch 'main' into pythongh-82378-fix-tracebacklimit-in-pyrepl
01cb96a
add news entry
2e18e4c
Merge branch 'main' into pythongh-82378-fix-tracebacklimit-in-pyrepl
b741cb4
simplify the code again to 3.12
a9703a1 
remove the undocumented new keyword arguments colorize and limit that
were added only for pyrepl
bedevere-app Bot mentioned this pull request Aug 16, 2024
Closed
cfbolz mentioned this pull request Aug 16, 2024
Closed
cfbolz added a commit to pypy/pypy that referenced this pull request Aug 16, 2024
take over the new approach in code.py, from python/cpython#123062
1ebb72a 
this removes the extra keyword argument colorize in showtraceback and
showsyntaxerror again, which should hopefully fix the problems in
gh-5004.
serhiy-storchaka reviewed Aug 16, 2024
View reviewed changes

serhiy-storchaka 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

I feel discomfort both from the current code and from this PR.

The current code violates the Liskov substitution principle: the signatures of showsyntaxerror() and showtraceback() in subclasses are not compatible with signatures in the base class.

In this PR the overridden _showtraceback() has compatible signature, but ignores its arguments. And what if we will need to pass additional arguments to format_exception() in a subclass? We will need to modify the base class.

Comment thread Lib/test/test_pyrepl/test_pyrepl.py Outdated
Comment thread Lib/test/test_pyrepl/test_pyrepl.py Outdated
Comment thread Lib/code.py
Comment thread Lib/code.py Outdated

cfbolz commented Aug 16, 2024

Copy link
Copy Markdown
Contributor Author

@serhiy-storchaka thanks for your super thoughtful comments. I share your sense of discomfort, there's definitely too tight coupling between InteractiveConsole and pyrepl. I'll try to implement something along the lines of your suggestion. Since you aren't objecting to the removal of the undocumented arguments, I'll close #122452 ;-).

cfbolz commented Aug 16, 2024

Copy link
Copy Markdown
Contributor Author

@serhiy-storchaka I did your suggestions, let me know what you think.

serhiy-storchaka approved these changes Aug 16, 2024
View reviewed changes

serhiy-storchaka 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.

Comment thread Lib/test/test_pyrepl/test_pyrepl.py Outdated
Comment thread Lib/test/test_pyrepl/test_pyrepl.py Outdated

cfbolz commented Aug 16, 2024

Copy link
Copy Markdown
Contributor Author

@serhiy-storchaka thanks for all your help!

pablogsal merged commit 63603bc into python:main Aug 18, 2024
pablogsal added the needs backport to 3.13 bugs and security fixes label Aug 18, 2024

miss-islington-app Bot commented Aug 18, 2024

Copy link
Copy Markdown

Thanks @cfbolz for the PR, and @pablogsal for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

miss-islington-app Bot commented Aug 18, 2024

Copy link
Copy Markdown

Sorry, @cfbolz and @pablogsal, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 63603bca35798c166e1b8e0be76aef69217f8b1b 3.13

cfbolz added a commit to cfbolz/cpython that referenced this pull request Aug 18, 2024
[3.13] pythongh-82378 fix sys.tracebacklimit in pyrepl, approach 2 (p…
6723780 
…ythonGH-123062)

Make sure that pyrepl uses the same logic for sys.tracebacklimit as both
the basic repl and the standard sys.excepthook
(cherry picked from commit 63603bc)

Co-authored-by: CF Bolz-Tereick <cfbolz@gmx.de>
jeremyhylton pushed a commit to jeremyhylton/cpython that referenced this pull request Aug 19, 2024
pythongh-82378 fix sys.tracebacklimit in pyrepl, approach 2 (python#1…
29e198c 
…23062)

Make sure that pyrepl uses the same logic for sys.tracebacklimit as both
the basic repl and the standard sys.excepthook
blhsing pushed a commit to blhsing/cpython that referenced this pull request Aug 22, 2024
pythongh-82378 fix sys.tracebacklimit in pyrepl, approach 2 (python#1…
c0de095 
…23062)

Make sure that pyrepl uses the same logic for sys.tracebacklimit as both
the basic repl and the standard sys.excepthook
cfbolz added a commit to cfbolz/cpython that referenced this pull request Aug 22, 2024
[3.13] pythongh-82378 fix sys.tracebacklimit in pyrepl, approach 2 (p…
af3bb21 
…ythonGH-123062)

Make sure that pyrepl uses the same logic for sys.tracebacklimit as both
the basic repl and the standard sys.excepthook
(cherry picked from commit 63603bc)

Co-authored-by: CF Bolz-Tereick <cfbolz@gmx.de>
cfbolz mentioned this pull request Aug 22, 2024
Merged
cfbolz added a commit to cfbolz/cpython that referenced this pull request Aug 23, 2024
[3.13] pythongh-82378 fix sys.tracebacklimit in pyrepl, approach 2 (p…
b847597 
…ythonGH-123062)

Make sure that pyrepl uses the same logic for sys.tracebacklimit as both
the basic repl and the standard sys.excepthook
(cherry picked from commit 63603bc)

Co-authored-by: CF Bolz-Tereick <cfbolz@gmx.de>
cfbolz mentioned this pull request Aug 23, 2024
Merged

bedevere-app Bot commented Aug 23, 2024

Copy link
Copy Markdown

GH-123252 is a backport of this pull request to the 3.13 branch.

bedevere-app Bot removed the needs backport to 3.13 bugs and security fixes label Aug 23, 2024
serhiy-storchaka added the needs backport to 3.12 only security fixes label Aug 23, 2024

miss-islington-app Bot commented Aug 23, 2024

Copy link
Copy Markdown

Thanks @cfbolz for the PR, and @pablogsal for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

miss-islington-app Bot commented Aug 23, 2024

Copy link
Copy Markdown

Sorry, @cfbolz and @pablogsal, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 63603bca35798c166e1b8e0be76aef69217f8b1b 3.12

pablogsal pushed a commit that referenced this pull request Aug 23, 2024
[3.13] gh-82378 fix sys.tracebacklimit in pyrepl, approach 2 (GH-123062…
0955db1 
…) (#123252)

Make sure that pyrepl uses the same logic for sys.tracebacklimit as both
the basic repl and the standard sys.excepthook
(cherry picked from commit 63603bc)

bedevere-app Bot commented Aug 23, 2024

Copy link
Copy Markdown

GH-123252 is a backport of this pull request to the 3.13 branch.

pablogsal changed the title gh-82378 fix sys.tracebacklimit in pyrepl, approach 2 gh-82378 fix sys.tracebacklimit in pyrepl Aug 23, 2024

cfbolz commented Aug 24, 2024

Copy link
Copy Markdown
Contributor Author

it doesn't really make sense to backport this to 3.12, because it's about pyrepl, does it? it still has the "needs backport" label.

hugovk removed the needs backport to 3.12 only security fixes label Feb 19, 2025
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

serhiy-storchaka serhiy-storchaka approved these changes
pablogsal Awaiting requested review from pablogsal pablogsal is a code owner
lysnikolaou Awaiting requested review from lysnikolaou lysnikolaou is a code owner
ambv Awaiting requested review from ambv ambv is a code owner

Assignees

pablogsal

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL