-
Notifications
You must be signed in to change notification settings - Fork 805
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
Format python traceback in impl Debug for PyErr. #4900
Conversation
48a7bf4
to
a9ec2c0
Compare
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.
Thanks! This looks reasonable to me. Just a quick thought before landing this: Does it make sense to keep the inner Result
here, or should we flatten it into the outer Option
for simplicity?
I guess if it errors we would try to render the error and maybe get into an infinite descent? I suggest we flatten and use logic similar to the |
Sorry, I meant Line 484 in 2c732a7
|
a9ec2c0
to
f6e8e0d
Compare
Done. I thought about using tb.format().map_err() instead of the match but the match is more readable IMO. This gives output like Is there a good way to test this? |
CodSpeed Performance ReportMerging #4900 will not alter performanceComparing Summary
|
src/err/mod.rs
Outdated
Err(err) => { | ||
err.write_unraisable(py, Some(&tb)); | ||
format!("<unformattable {:?}>", tb) | ||
} |
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 we construct an unraisable traceback in a test to have coverage here?
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.
Don't know, that's what I meant in my last reply with "Is there a good way to test this?". If anyone here knows how I can construct an unformattable traceback, I'm happy to add a test for 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.
I could do something like mock out io.StringIO in the python, that should cause the format to fail, not sure if we want to go down that route.
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.
Added a test with that approach.
f6e8e0d
to
d3c1782
Compare
d3c1782
to
9546f5f
Compare
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.
Looks good to me now, thank you very much!
Fixes #4863.