bpo-45292: [PEP 654] Update traceback display code to work with exception groups#29207
bpo-45292: [PEP 654] Update traceback display code to work with exception groups#29207iritkatriel merged 27 commits into
Conversation
|
This is almost exactly the same as I used for the PEP - I just removed the "with X sub-exceptions" line from the tracebacks because I think it clutters them and is not very useful. |
Sorry, something went wrong.
Just be sure to update the PEP examples. :-) |
Sorry, something went wrong.
|
🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit 2052c77 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
Sorry, something went wrong.
|
When you're done making the requested changes, leave the comment: |
Sorry, something went wrong.
erlend-aasland
left a comment
There was a problem hiding this comment.
First round comments :) I'll take a new round on references and pointers later.
Sorry, something went wrong.
I think it's OK to always box the EGs because they're pretty special, although I'd change the very first line of the output, from: to: |
Sorry, something went wrong.
|
@1st1 - see below.
The indentation of the next line is wrong. Is this a copy-paste error or a bug?
|
Sorry, something went wrong.
Copy/paste/whitespace issue; no implementation bugs were observed! |
Sorry, something went wrong.
gvanrossum
left a comment
There was a problem hiding this comment.
I like the max_group_{with,depth} solution and the refactor of the chaining loop!
I didn't review the C code carefully, it looks like others are on that.
The only things left from me are nits in the formatting of some of the overflow messages. Otherwise LGTM!
Sorry, something went wrong.
|
🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit 61fab3f 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
Sorry, something went wrong.
gvanrossum
left a comment
There was a problem hiding this comment.
LG!
Sorry, something went wrong.
Thank you! @gpshead @erlend-aasland @1st1 I'll wait a bit before merging in case you want to have another look. |
Sorry, something went wrong.
There was a problem hiding this comment.
Looks good; great job!
The only remark I have is that I find the error handling verbose and hard to read. I'm used to (and prefer):
int
func()
{
int rc = first();
if (rc < 0) {
goto error;
}
int err = second();
if (err != 0) {
goto error;
}
// etc.
// at the end:
return 0;
error:
cleanup();
return -1;
}The current approach of always verifying that the previous step did not fail seems to lead to a lot of indenting levels. I could count 7 (!) indents in one of the functions.
I would consider simplifying the error handling for the sake of readability.
Feel free to disregard this comment; It's only personal preference :)
Sorry, something went wrong.
|
@erlend-aasland I agree with your comment. I tried using gotos but it was a bit delicate because you need to make sure the refcounting is still correct. This is a refactor that should be done in its own PR and not together with major functional changes. The current method preserves the control flow so it doesn't impact refcounts. |
Sorry, something went wrong.
+1 🙂 |
Sorry, something went wrong.
|
Thank you everyone for your help! |
Sorry, something went wrong.
https://bugs.python.org/issue45292