-
-
Notifications
You must be signed in to change notification settings - Fork 33.1k
gh-139551: add support for BaseExceptionGroup in IDLE #139563
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
base: main
Are you sure you want to change the base?
gh-139551: add support for BaseExceptionGroup in IDLE #139563
Conversation
The change that |
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.
Can you add tests and a What's New entry as well? I think the code could also avoid having all those if prefix:
inside the for loops and many times, the additional string is either two spaces or the given prefix.
Lib/idlelib/run.py
Outdated
"another exception occurred:\n", file=efile) | ||
if isinstance(exc, BaseExceptionGroup): | ||
if tb: | ||
if not prefix: |
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.
Let's swap the if
to be consistent with the other ifs.
Misc/NEWS.d/next/IDLE/2025-10-04-22-34-06.gh-issue-139551.hJDxw6.rst
Outdated
Show resolved
Hide resolved
Lib/idlelib/run.py
Outdated
else: | ||
first_line_pre = " " | ||
if not prefix: | ||
print(f" {first_line_pre}+---------------- {i} ----------------", file=efile) |
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 would prefer that -
is stored as a variable, say sep = '-' * N
where N
is the number of '-' you want to print. You would also be able to use "prefix2".
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.
The code in traceback.py use many "-" to print instead of sep = '-' * N
: (for example now in 3.13.7)
# format exception group
is_toplevel = (_ctx.exception_group_depth == 0)
if is_toplevel:
_ctx.exception_group_depth += 1
if exc.stack:
yield from _ctx.emit(
'Exception Group Traceback (most recent call last):\n',
margin_char = '+' if is_toplevel else None)
yield from _ctx.emit(exc.stack.format(colorize=colorize))
yield from _ctx.emit(exc.format_exception_only(colorize=colorize))
num_excs = len(exc.exceptions)
if num_excs <= self.max_group_width:
n = num_excs
else:
n = self.max_group_width + 1
_ctx.need_close = False
for i in range(n):
last_exc = (i == n-1)
if last_exc:
# The closing frame may be added by a recursive call
_ctx.need_close = True
if self.max_group_width is not None:
truncated = (i >= self.max_group_width)
else:
truncated = False
title = f'{i+1}' if not truncated else '...'
yield (_ctx.indent() +
('+-' if i==0 else ' ') +
f'+---------------- {title} ----------------\n')
_ctx.exception_group_depth += 1
if not truncated:
yield from exc.exceptions[i].format(chain=chain, _ctx=_ctx, colorize=colorize)
else:
remaining = num_excs - self.max_group_width
plural = 's' if remaining > 1 else ''
yield from _ctx.emit(
f"and {remaining} more exception{plural}\n")
if last_exc and _ctx.need_close:
yield (_ctx.indent() +
"+------------------------------------\n")
_ctx.need_close = False
_ctx.exception_group_depth -= 1
if is_toplevel:
assert _ctx.exception_group_depth == 1
_ctx.exception_group_depth = 0
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.
Then I would like you to actually split the lines as it's done, not have everything on a single line (as you can see each group containing -
is on its own line).
seen = set() | ||
|
||
def print_exc(typ, exc, tb): | ||
def print_exc(typ, exc, tb, prefix=""): |
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.
Since this is anyway a local function, I'd suggest having other helpers to reduce the complexity of the function.
else: | ||
print("\nDuring handling of the above exception, " | ||
"another exception occurred:\n", file=efile) | ||
if isinstance(exc, BaseExceptionGroup): |
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.
For instance, here, I would suggest using a separate function, say print_exc_group()
that you would define locally as well.
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.
The change that if isinstance(val, BaseExceptionGroup) branch becomes a function alone is difficult because it is a closure function.
Even with that, it's not really important. What is the issue with it being a local function? print_exc
is already a local function. Just pass the arguments you need to that local function.
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
More generally, don't we have a helper in |
Maybe not. In IDLE, it need |
Ok. For now, let's write it as is, and later we can rewrite the exception rendering logic by using the same approach as for |
I have no idea whether to do it. What should be used is |
No, I meant, we should mimic this ourselves. We may need to tweak the context itself, but the approach can be taken. Not in this PR though I would say as it may be too much changes in one PR. |
Uh oh!
There was an error while loading. Please reload this page.