Skip to content
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

bpo-46729: improved str() for BaseExceptionGroup #31294

Merged
merged 4 commits into from
Feb 22, 2022

Conversation

iritkatriel
Copy link
Member

@iritkatriel iritkatriel commented Feb 12, 2022

Draft PR to discuss options for a better Exception Group str().

Initial suggestion is to add "(group of X exceptions)", but this seems a bit too long.

https://bugs.python.org/issue46729

@iritkatriel
Copy link
Member Author

@1st1 See #31270 (comment) .

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

It feels weird to report the number of nodes in the tree without any indication of the tree structure. I would be okay if the number just was the count of the immediate subexceptions (i.e., len(eg.exceptions)). If you want to show the number of leaves: (a) don't count interior nodes!; (b) add something to the message indicating that the tree isn't flat (if it isn't). Maybe make the message "N sub-exceptions" if N == len(eg.exceptions) but "N sub-exceptions in tree of depth D" if there are sub-sub (etc.) exceptions?

@iritkatriel
Copy link
Member Author

It feels weird to report the number of nodes in the tree without any indication of the tree structure. I would be okay if the number just was the count of the immediate subexceptions (i.e., len(eg.exceptions)). If you want to show the number of leaves: (a) don't count interior nodes!; (b) add something to the message indicating that the tree isn't flat (if it isn't). Maybe make the message "N sub-exceptions" if N == len(eg.exceptions) but "N sub-exceptions in tree of depth D" if there are sub-sub (etc.) exceptions?

I'd go with just the number of directly contained exceptions, because a nested group is really just one error from some group operation.

But I wonder if any count is really useful information here. We said iteration is irrelevant because it's not about how many but about what types of exceptions are included. Maybe we should drop the number and indicate in some other way that this is a group of exceptions?

@gvanrossum
Copy link
Member

Yury’s MultiError had the set of exception names in the message (as well as the count). Maybe that’s an idea?

@gvanrossum
Copy link
Member

That could be for the entire tree (leaves only), OR only the first level (so it might show EG in there). If too many, use ‘…’ .

@iritkatriel
Copy link
Member Author

Yury’s MultiError had the set of exception names in the message (as well as the count). Maybe that’s an idea?

Yury had the whole traceback in str(), right?

The str() appears in the standard traceback, so the types would be repetitive.

Also we would want to dedup them, so we would need to construct a set of the types. We don’t want to allocate memory while rendering an exception, so we will need to do this in the constructor and save the set.

@gvanrossum
Copy link
Member

Yury’s MultiError had the set of exception names in the message (as well as the count). Maybe that’s an idea?

Yury had the whole traceback in str(), right?

Yeah, but the first line has this appended the the message argument: "N sub errors: (TYPES)" where N is len(errors) and TYPES is the set of error types -- both only using the immediate child level.
see here.

The str() appears in the standard traceback, so the types would be repetitive.

But the traceback is pretty verbose -- the first line of the message contains a useful summary. (Admittedly mostly used by tests.)

Also we would want to dedup them, so we would need to construct a set of the types. We don’t want to allocate memory while rendering an exception, so we will need to do this in the constructor and save the set.

Or the string -- perhaps more work but a simpler type.

Then again maybe we should just stick with what we have.

@iritkatriel
Copy link
Member Author

On second thought I don’t think it’s a problem to put the number as a kind of summary and indication that it’s a group.

The types appear in the repr() so maybe that’s enough.

In the original implementation str() just printed the msg and then in the traceback we added a line under the msg like “with x sub-exceptions”. We removed that later because we thought it was clutter. It makes more sense to put it on the exception str() for when that’s used in other contexts, and also it doesn’t add a line to the traceback.

@1st1 do you have a view?

@gvanrossum
Copy link
Member

If the str() would just be f"{self.message} ({len(self.exceptions)} sub-exceptions)" that would be nice. Agreed the repr() is good as it is.

@iritkatriel
Copy link
Member Author

I made a draft PR for the PEP update: python/peps#2356
I don't think we ever specified str() and repr() in the PEP, it's just implied from the examples.

Should I now ask the SC to approve it?

@iritkatriel iritkatriel marked this pull request as ready for review February 22, 2022 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants