bpo-31299: Make it possible to filter out frames from tracebacks by iritkatriel · Pull Request #26772 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation23 Commits7 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new parameter must be added to most or all of the format/print functions that call TracebackException.
I did not look at tests.
When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.
bpo-31299: Add the hide_frames option to traceback.TracebackException bpo-31299: Add the frame_filter option to traceback.TracebackException
Apart from adding the new arg to the module level wrapper functions (which I'm unsure about - see #26772 (comment)) ...
I have made the requested changes; please review again
Thanks for making the requested changes!
@terryjreedy: please review the changes made to this pull request.
The new parameter must be added to most or all of the format/print functions that call TracebackException.
Do you think we should change those functions to take any **kwargs and forward them to the TracebackException constructor?
So to change the signature from
def print_exception(exc, /, value=_sentinel, tb=_sentinel, limit=None, \
file=None, chain=True):
to something like
def print_exception(exc, /, value=_sentinel, tb=_sentinel, \
file=None, chain=True, **kwargs):
?
(file and chain go into print() rather than init).
I changed this to use the refactor of #27038
bpo-31299: Add the frame_filter option to traceback.TracebackException bpo-31299: Make it possible to filter out frames from tracebacks
| @@ -618,7 +626,7 @@ class TracebackException: |
|---|
| def __init__(self, exc_type, exc_value, exc_traceback, *, limit=None, |
| lookup_lines=True, capture_locals=False, compact=False, |
| _seen=None): |
| stack_summary_cls=None, _seen=None): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a good approach to customizing the behavior while reusing most of the exception printing code. There's precedent for this style of expansion with JSONEncoder and cls in the json module. Any thoughts @terryjreedy?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See general review comment.
Co-authored-by: Ammar Askar ammar_askar@hotmail.com
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR does two things: 1. Expand the API of new format_frame function by handing None returns. Returning '' and appending it to result does not completely work as it adds a blank line to the end output. 2. Expand API of Traceback Exception to allow using a custom format_frame in a subclass of StackSummary rather than monkeypatching it into the original. This is generally preferable.
I regard this PR as completing what we started in #27038. When I proposed extracting format_frame, I was hoping that it could be an alternative to adding yet another keyword parameter* to multiple module-level convenience functions, as was originally considered here. I am delighted to see this happen. I am OK with not adding the parameter to the functions. I have since decided that people with idiosyncratic needs should learn to call TracebackException (starting with me ;-).
- For this issue, there was discussion about whether the new parameter should be general -- a boolean filter function (called with what?) -- or specific, a filter set used by a hard-coded filter function.
I presume that the new last_line_displayed code was shown to be necessary and sufficient by the tests. This time, I read the new tests and they look good. My only suggestion is the one about the doc.
| If *stack_summary_cls* is not ``None``, it is a class to be used instead of |
|---|
| the default :class:`~traceback.StackSummary` to format the stack (typically |
| a subclass that overrides :meth:`~traceback.StackSummary.format_frame`). |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a code example, in particular, class SkipG from the test, which skips frames for a function named 'g'.
| @@ -618,7 +626,7 @@ class TracebackException: |
|---|
| def __init__(self, exc_type, exc_value, exc_traceback, *, limit=None, |
| lookup_lines=True, capture_locals=False, compact=False, |
| _seen=None): |
| stack_summary_cls=None, _seen=None): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See general review comment.
Thank you @ammaraskar and @terryjreedy for the reviews.
I presume that the new last_line_displayed code was shown to be necessary and sufficient by the tests.
Actually no, it was from my reading of the code - I wanted to prevent printing something about "the previous line" when the previous line was not printed. But I should have added a test for this, and I will do that shortly.
Consider adding a code example, in particular, class SkipG from the test, which skips frames for a function named 'g'.
I agree, in fact the are no examples at all for using TracebackException directly, and there is an open issue regarding that (Issue27597). I want to tackle that next and rewrite the TracebackException part of the doc to explain why and how it should be used. I'll send it for you to review once I have it.
I added some more tests, and found two bugs in the process.
- There are two places where recursion is detected so I needed to suppress the "Previous line..." message in another place.
- I didn't pass the stack_summary_class to the recursive construction of TracebackExceptions for
__cause__and__context__. This is fixed now and covered by tests.
I made the doc updates as part of PR iritkatriel#19 (which is against the current branch).
I have made the requested changes; please review again
Thanks for making the requested changes!
@terryjreedy: please review the changes made to this pull request.
…one' and then we don't need the last_line_displayed flag
It seems that this got overly complicated. I've created PR28067 to just handle the Non return (without adding this to the TracebackException API). That will allow us to use this for the unittest bug, and we can decide later about the external API.