ExceptionGroup PEP654 proof of concept. #3033
Conversation
|
@willmcgugan, when you have some time, could you share some of your critique so I can flesh this out and we can hopefully get this landed? Would like to get some early feedback before going the whole mile. |
| @@ -417,6 +426,20 @@ def safe_str(_object: Any) -> str: | |||
| msg=exc_value.msg, | |||
| ) | |||
|
|
|||
|
|
|||
| if isinstance(exc_value, BaseExceptionGroup): | |||
There was a problem hiding this comment.
It would be very easy to make this support older Python versions, with or without the exceptiongroup backport:
| if isinstance(exc_value, BaseExceptionGroup): | |
| if sys.version_info[:2] < (3, 11): | |
| BaseExceptionGroup = getattr(sys.modules.get("exceptiongroup"), "BaseExceptionGroup", ()) | |
| if isinstance(exc_value, BaseExceptionGroup): |
There was a problem hiding this comment.
Yes, this was the plan but hasn't been implemented because I wanted to get a PoC out first with some feedback. See my first post as well at the bottom.
| elif stack.grouped_traces: | ||
| group_exception_renderable = Panel( | ||
| Group(*[Group(*self._render_trace(console=console, options=options, trace=trace, child_index=index + 1)) for index, trace in enumerate(stack.grouped_traces)]), | ||
| title=f"[exception_group.title]{stack.exc_type} [dim]{highlighter(stack.exc_value)}", |
There was a problem hiding this comment.
I'd love to see support for PEP-678 __notes__ here (and in the next elif clause, for non-grouped exceptions). Probably easiest if we add a new method .render_exc_value(stack) cribbing from this part of the backport?
There was a problem hiding this comment.
Also wanted to do this, but again wanted to get initial feedback first as this will depend on how the maintainer was possibly envisioning how it'd fit in the "design". If I have the go ahead for how it should be achieved, I can make the changes. Right now we're stuck waiting for feedback.
There was a problem hiding this comment.
My intuition as a maintainer is that fewer rounds of review makes everything easier - if I can just say "thanks, merging" that's likely to happen sooner than if we iterate. I understand not wanting to do more work which might not be merged though. 🙂
|
@willmcgugan, do you happen to have some time to give this a look? Marking it as ready for review even though it was my plan to get some initial feedback first. There doesn't seem to be bandwidth and that's okay, though a quick message would be nice on what we could do to get this improved. |
|
Just busy ATM. I will do a pass on Rich soon. Initial impressions are that it is a good idea. |
|
Hi @AndreasBackx Sorry for the slow turnaround. Yes, this is great in principle. Re styling, I like the borders, but some don't and I expect they would complain. Maybe an indent would be better to show the exception groups as in the default? Worth experimenting. I am going to do another pass on Rich next month. I could tackle this then. But happy for you to continue, if you still have the time? |
|
@willmcgugan if you're giving this a pass anyhow. Go ahead and take it over. That will be easier than communicating around this. Let me know if you need any feedback. Regarding the borders, I would prefer a version where no frames/borders were used. Why? Because we're using Rich for logging in places where there is no set terminal width and so avoid the frames as they don't show up nicely when the output is saved and shown in a different location: someone else's terminal or the browser. |
Type of changes
Checklist
Description
This is just a proof of concept for how exception groups may look, primarily looking to get some feedback on:
This implementation does not (yet) care about backwards-compatibility or supportng the exceptiongroup pypi backport, that is to be added later. This is how the current example I've added looks when running,
Compared to the default (which is also printed below it):
This resolves #1859.