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

Enable rendering of rich exception objects in traceback #3325

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

pradyunsg
Copy link
Contributor

@pradyunsg pradyunsg commented Apr 7, 2024

Type of changes

  • Bug fix
  • New feature
  • Documentation / docstrings
  • Tests
  • Other

Checklist

  • I've run the latest black with default args on new code.
  • I've updated CHANGELOG.md and CONTRIBUTORS.md where appropriate.
  • I've added tests for new code.
  • I accept that @willmcgugan may be pedantic in the code review.

Description

See #2717 for discussion. This enables exceptions to be rich objects and be rendered via the rich rendering pipeline, in tracebacks. This can be tested manually by running python -m rich.traceback whose example now uses a rich object instead of the NameError.

@pradyunsg pradyunsg marked this pull request as draft April 7, 2024 19:32
@pradyunsg
Copy link
Contributor Author

This is currently a draft since I'm not sure how exactly exceptions within the rich object rendering should be handled -- I'm leaning toward trying to catch them and presenting a fallback like "render failed" or similar. Not quite sure how to do that, so figured I'd open this as a draft PR in case y'all have any opinions/suggestions on how to achieve that.

@pradyunsg
Copy link
Contributor Author

OK, I think I figured out something that works! 🎉

Using the Console object to eagerly render the exception and iterating through to collect all the segments, before yielding those segments forward. This enables this logic to catch any rendering-related issues inline, rather than needing to handle them at some other level (eg: console.render).

Marking this as ready for review, since I think everything that is necessary to land this is in place now1 -- code, tests, documentation, changelog, contributing.md. :)

Footnotes

  1. Assuming of course that there are no concerns with the implementation/approach. 😉

@pradyunsg pradyunsg marked this pull request as ready for review April 7, 2024 20:01
@pradyunsg pradyunsg force-pushed the rich-exceptions-in-traceback branch from 9e34085 to 9cb8a34 Compare April 7, 2024 20:04
@@ -722,7 +739,11 @@ def render_locals(frame: Frame) -> Iterable[ConsoleRenderable]:
from .console import Console

console = Console()
import sys
Copy link
Contributor Author

@pradyunsg pradyunsg Apr 7, 2024

Choose a reason for hiding this comment

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

sys was unused at this point, and there's already a top-level import sys.

@pradyunsg
Copy link
Contributor Author

Oh, and here's an SVG showing that this works!

exception

except Exception:
console.print_exception()
result = console.file.getvalue()
print(result)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These prints are useful for diagnosing failures, and pytest will capture this output appropriately. :)

Copy link
Collaborator

@willmcgugan willmcgugan left a comment

Choose a reason for hiding this comment

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

I can see why this would be nice, but it breaks some assumptions. See comment...

@@ -416,6 +418,8 @@ def safe_str(_object: Any) -> str:
line=exc_value.text or "",
msg=exc_value.msg,
)
if isinstance(exc_value, RichRenderable):
Copy link
Collaborator

Choose a reason for hiding this comment

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

My concern here is that if we offer arbitrary renderables, then the Traceback object may no longer be trivially sererializeable. Which was one of the goals for this class.

And since a renderable may be mutable, it may become out of date if it isn't immediately printed.

@willmcgugan
Copy link
Collaborator

@pradyunsg Any thoughts on above? The issue with serialization may be a deal breaker.

@pradyunsg
Copy link
Contributor Author

pradyunsg commented Aug 26, 2024

Thanks for the mention -- I'd missed the original review.

My concern here is that if we offer arbitrary renderables, then the Traceback object may no longer be trivially sererializeable.

(I assume by serialization you mean [object] -> [rich-rendered string] and not some on-disk/pickle representation or something)

I share that, which is why there's a fallback to the string-based approach in the render side of this if the serialization via the rich protocol fails. I think it's an acceptable tradeoff since this is opt-in on the exception raiser's end as this needs an additional set of methods on exception class itself.

I'd be fine with adding another layer of opt-in boolean on traceback.install for rendering such exceptions from the caller's end as well, if you think it's important enough to have that for some reason (eg: there are usecases for not wanting to do the render in some cases other than the ones covered by the fallback logic).

And since a renderable may be mutable, it may become out of date if it isn't immediately printed.

That's also the case with exception objects themselves and wouldn't be unique to rich's renderables (__str__ and other methods on an exception object can mutate itself in a similar manner as __rich__ can).

cat /tmp/foo.py           
try:
    raise Exception("This is an error message")
except Exception as e:
    print(e)
    e.args = ("This is a new error message",)
    raisepython /tmp/foo.py
This is an error message
Traceback (most recent call last):
  File "/tmp/foo.py", line 2, in <module>
    raise Exception("This is an error message")
Exception: This is a new error message

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.

2 participants