GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-82378 fix sys.tracebacklimit in pyrepl by cfbolz · Pull Request #123062 · python/cpython

Conversation

@cfbolz

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

Uh oh!

There was an error while loading. Please reload this page.

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?

  • Issue: Meaning of tracebacklimit differs between sys.tracebacklimit and traceback module #82378

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
cfbolz added 5 commits July 30, 2024 14:01
@cfbolz
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
@cfbolz
Merge branch 'main' into pythongh-82378-fix-tracebacklimit-in-pyrepl
01cb96a
@cfbolz
add news entry
2e18e4c
@cfbolz
Merge branch 'main' into pythongh-82378-fix-tracebacklimit-in-pyrepl
b741cb4
@cfbolz
simplify the code again to 3.12
a9703a1 
remove the undocumented new keyword arguments colorize and limit that
were added only for pyrepl
@cfbolz
cfbolz requested review from ambv, lysnikolaou and pablogsal as code owners August 16, 2024 08:22
@bedevere-app bedevere-app Bot added the awaiting core review label Aug 16, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Aug 16, 2024
Meaning of tracebacklimit differs between sys.tracebacklimit and traceback module #82378
Closed
@cfbolz cfbolz mentioned this pull request Aug 16, 2024
gh-82378: fix sys.tracebacklimit in pyrepl #122452
Closed
cfbolz added a commit to pypy/pypy that referenced this pull request Aug 16, 2024
@cfbolz
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
serhiy-storchaka reviewed Aug 16, 2024
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_pyrepl/test_pyrepl.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_pyrepl/test_pyrepl.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/code.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/code.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@cfbolz

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 ;-).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
cfbolz added 2 commits August 16, 2024 22:22
@cfbolz
add another method to make things less weird from an OO perspective
e736e73
@cfbolz
test both presence and absence of tracebacklimit
482f74e
@cfbolz

cfbolz commented Aug 16, 2024

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka
serhiy-storchaka approved these changes Aug 16, 2024
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_pyrepl/test_pyrepl.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_pyrepl/test_pyrepl.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Aug 16, 2024
cfbolz added 3 commits August 16, 2024 22:52
@cfbolz
remove duplication by adding another loop
7116f41
@cfbolz
remove \
5202d27
@cfbolz
Merge branch 'main' into pythongh-82378-fix-tracebacklimit-in-pyrepl-…
fd09638 
…simplification
@cfbolz

cfbolz commented Aug 16, 2024

Copy link
Copy Markdown
Contributor Author

@serhiy-storchaka thanks for all your help!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka
serhiy-storchaka approved these changes Aug 17, 2024
View reviewed changes
@pablogsal
pablogsal merged commit 63603bc into python:main Aug 18, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Aug 18, 2024
@pablogsal pablogsal added the needs backport to 3.13 only security fixes label Aug 18, 2024
@miss-islington-app

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!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington-app

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington-app miss-islington-app Bot assigned pablogsal Aug 18, 2024
cfbolz added a commit to cfbolz/cpython that referenced this pull request Aug 18, 2024
@cfbolz
[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 <[email protected]>
jeremyhylton pushed a commit to jeremyhylton/cpython that referenced this pull request Aug 19, 2024
@cfbolz @jeremyhylton
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
@cfbolz @blhsing
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
@cfbolz
[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 <[email protected]>
@cfbolz cfbolz mentioned this pull request Aug 22, 2024
gh-122546: use same filename for different exceptions in new repl #123217
Merged
cfbolz added a commit to cfbolz/cpython that referenced this pull request Aug 23, 2024
@cfbolz
[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 <[email protected]>
@cfbolz cfbolz mentioned this pull request Aug 23, 2024
[3.13] gh-82378 fix sys.tracebacklimit in pyrepl (GH-123062) #123252
Merged
@bedevere-app

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot removed the needs backport to 3.13 only security fixes label Aug 23, 2024
@serhiy-storchaka serhiy-storchaka added the needs backport to 3.12 only security fixes label Aug 23, 2024
@miss-islington-app

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.
🐍🍒⛏🤖
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington-app

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
pablogsal pushed a commit that referenced this pull request Aug 23, 2024
@cfbolz
[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

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pablogsal pablogsal changed the title gh-82378 fix sys.tracebacklimit in pyrepl, approach 2 gh-82378 fix sys.tracebacklimit in pyrepl Aug 23, 2024
@cfbolz

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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@hugovk hugovk removed the needs backport to 3.12 only security fixes label Feb 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@pablogsal pablogsal

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@cfbolz @serhiy-storchaka @hugovk @pablogsal
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.