bpo-31299: Make it possible to filter out frames from tracebacks #26772

iritkatriel · 2021-06-17T16:11:45Z

https://bugs.python.org/issue31299

terryjreedy

The new parameter must be added to most or all of the format/print functions that call TracebackException.

I did not look at tests.

Doc/library/traceback.rst

bedevere-bot · 2021-06-22T14:13:34Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

Doc/library/traceback.rst

iritkatriel · 2021-06-23T14:55:31Z

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

bedevere-bot · 2021-06-23T14:55:34Z

Thanks for making the requested changes!

@terryjreedy: please review the changes made to this pull request.

Lib/traceback.py

iritkatriel · 2021-07-03T11:24:25Z

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).

iritkatriel · 2021-07-16T18:50:04Z

I changed this to use the refactor of #27038

Doc/library/traceback.rst

ammaraskar · 2021-07-16T19:41:51Z

Lib/traceback.py

@@ -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):


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?

See general review comment.

Lib/test/test_traceback.py

Co-authored-by: Ammar Askar <ammar_askar@hotmail.com>

terryjreedy

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.

terryjreedy · 2021-07-16T20:46:13Z

Doc/library/traceback.rst

+   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`).
+


Consider adding a code example, in particular, class SkipG from the test, which skips frames for a function named 'g'.

terryjreedy · 2021-07-16T21:08:30Z

Lib/traceback.py

@@ -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):


See general review comment.

iritkatriel · 2021-07-18T19:23:24Z

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.

iritkatriel · 2021-07-19T00:05:30Z

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.

iritkatriel · 2021-07-19T20:38:33Z

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

bedevere-bot · 2021-07-19T20:38:36Z

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

iritkatriel · 2021-08-30T13:51:19Z

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.

the-knights-who-say-ni added the CLA signed label Jun 17, 2021

bedevere-bot added the awaiting core review label Jun 17, 2021

iritkatriel requested review from ncoghlan, pablogsal and terryjreedy June 17, 2021 16:12

terryjreedy requested changes Jun 22, 2021

View reviewed changes

Doc/library/traceback.rst Outdated Show resolved Hide resolved

bedevere-bot added awaiting changes and removed awaiting core review labels Jun 22, 2021

iritkatriel commented Jun 22, 2021

View reviewed changes

Doc/library/traceback.rst Outdated Show resolved Hide resolved

iritkatriel changed the title ~~bpo-31299: Add the hide_frames option to traceback.TracebackException~~ bpo-31299: Add the frame_filter option to traceback.TracebackException Jun 23, 2021

iritkatriel force-pushed the bpo-31299 branch from 4c81a26 to 7c251c6 Compare June 23, 2021 14:27

bedevere-bot added awaiting change review and removed awaiting changes labels Jun 23, 2021

bedevere-bot requested a review from terryjreedy June 23, 2021 14:55

iritkatriel commented Jun 23, 2021

View reviewed changes

Lib/traceback.py Outdated Show resolved Hide resolved

use StackSummary subclasses to filter frames

3830400

iritkatriel force-pushed the bpo-31299 branch from 7c251c6 to 3830400 Compare July 16, 2021 18:49

added documentation

4debb1d

iritkatriel changed the title ~~bpo-31299: Add the frame_filter option to traceback.TracebackException~~ bpo-31299: Make it possible to filter out frames from tracebacks Jul 16, 2021

iritkatriel requested a review from ammaraskar July 16, 2021 19:09

ammaraskar reviewed Jul 16, 2021

View reviewed changes

Update Doc/library/traceback.rst

caef03f

Co-authored-by: Ammar Askar <ammar_askar@hotmail.com>

terryjreedy reviewed Jul 16, 2021

View reviewed changes

more precise comparison in test

50fcdcc

add more tests and fix a couple of bugs

ac09f43

fix indent

68afed9

bedevere-bot requested a review from terryjreedy July 19, 2021 20:38

put all the recursive-cutoff logic under 'if formatted_frame is not N…

b025ee0

…one' and then we don't need the last_line_displayed flag

iritkatriel marked this pull request as draft August 28, 2021 20:19

iritkatriel closed this Aug 30, 2021

Uh oh!

bpo-31299: Make it possible to filter out frames from tracebacks #26772

bpo-31299: Make it possible to filter out frames from tracebacks #26772

Uh oh!

Conversation

iritkatriel commented Jun 17, 2021 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

terryjreedy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bedevere-bot commented Jun 22, 2021

Uh oh!

Uh oh!

iritkatriel commented Jun 23, 2021

Uh oh!

bedevere-bot commented Jun 23, 2021

Uh oh!

Uh oh!

iritkatriel commented Jul 3, 2021

Uh oh!

iritkatriel commented Jul 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ammaraskar Jul 16, 2021

Choose a reason for hiding this comment

Uh oh!

terryjreedy Jul 16, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

terryjreedy left a comment

Choose a reason for hiding this comment

Uh oh!

terryjreedy Jul 16, 2021

Choose a reason for hiding this comment

Uh oh!

terryjreedy Jul 16, 2021

Choose a reason for hiding this comment

Uh oh!

iritkatriel commented Jul 18, 2021

Uh oh!

iritkatriel commented Jul 19, 2021

Uh oh!

iritkatriel commented Jul 19, 2021

Uh oh!

bedevere-bot commented Jul 19, 2021

Uh oh!

iritkatriel commented Aug 30, 2021

Uh oh!

Uh oh!

iritkatriel commented Jun 17, 2021 •

edited by bedevere-bot

Loading

iritkatriel commented Jul 16, 2021 •

edited

Loading