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

bpo-42877: add the 'compact' param to TracebackException's __init__ #24179

Merged
merged 2 commits into from
Jan 15, 2021

Conversation

iritkatriel
Copy link
Member

@iritkatriel iritkatriel commented Jan 9, 2021

@iritkatriel
Copy link
Member Author

I don't know whether a change like this needs news.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Hm, I don't think you can make this change at all -- __cause__ and __context__ are documented and hence public APIs.

Maybe a different way to save space/effort is to set __context__ to None if there is a __cause__ -- but even that seems to violate the documented value for this attribute.

@iritkatriel
Copy link
Member Author

I see. I didn't realise all these fields are in the doc.

This class is probably mostly used from print_exception etc, we could add this as an opt-in optimisation to use from there. Is that worth doing?

@gvanrossum
Copy link
Member

This class is probably mostly used from print_exception etc, we could add this as an opt-in optimisation to use from there. Is that worth doing?

I don't know -- I guess you ran into this when solving the recursion error issue? I guess you could add a new keyword parameter skip_context=False which you can set to something like exc is not None and exc.__cause_ is not None_ at the call site? (Not sure if the exc is not None guard is needed.)

@iritkatriel
Copy link
Member Author

iritkatriel commented Jan 11, 2021

I'd pass in a bool (like compact or save_space) and leave the logic of whether to skip internet he class itself (note that there is also __suppress_context__).

Yes, this came up re the recursion issue. I'm still not sure what to do about that.

@gvanrossum
Copy link
Member

I'd pass in a bool (like compact or save_space) and leave the logic of whether to skip internet he class itself (note that there is also __suppress_context__).

Since the latter is set on the original exception we can't change it. Agreed to put as much logic in the constructor.

@gvanrossum
Copy link
Member

(This would also require a news item.)

@iritkatriel
Copy link
Member Author

Yes, and some new tests.

Since this won't simplify the code, it won't help deal with the recursion issue so it can come after PR 24158, which I've now refactored to use the fact that the chained exceptions are a list and not a tree.

@iritkatriel iritkatriel changed the title bpo-42877: TracebackException doesn't need to save __context__ if the… bpo-42877: add the 'compact' param to TracebackException's __init__ Jan 13, 2021
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Looks like you have a merge conflict due to your own #24158.

*capture_locals* are as for the :class:`StackSummary` class.
*capture_locals* are as for the :class:`StackSummary` class. If *compact*
is true, only data that is required for :method:`format` and
:method:`format_exception_only` is saved in the class attributes.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe clarify that this suppresses context if cause is not None?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, though I'll avoid "suppresses" so as not to confuse with __suppress_context__ which is applied in format().

I also don't need to mention format_exception_only because it's called from format.

Lib/traceback.py Outdated
and id(exc_value.__context__) not in _seen):

self.__suppress_context__ = \
exc_value.__suppress_context__ if exc_value else False
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
exc_value.__suppress_context__ if exc_value else False
exc_value.__suppress_context__ if exc_value is not None else False

(Some joker could add a __bool__ method to their exception that sometimes makes it falsey.)

…d use it to reduce the time and memory taken up by several of traceback's module-level functions
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Yup!

@gvanrossum gvanrossum merged commit 4c94d74 into python:master Jan 15, 2021
@bedevere-bot
Copy link

@gvanrossum: Please replace # with GH- in the commit message next time. Thanks!

@iritkatriel iritkatriel deleted the bpo-42877 branch January 15, 2021 10:05
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
…ython#24179)

Use it to reduce the time and memory taken up by several of traceback's module-level functions.
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.

4 participants