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.

Sign up for GitHub

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

Jump to bottom

bpo-42848: remove recursion from TracebackException #24158

Merged
merged 7 commits into from
Jan 12, 2021
Merged

bpo-42848: remove recursion from TracebackException #24158

merged 7 commits into from
Jan 12, 2021

Conversation

iritkatriel
Copy link
Member

@iritkatriel iritkatriel commented Jan 8, 2021 •
edited
Loading

@iritkatriel iritkatriel changed the title bpo 42848: remove recursion from TracebackException bpo-42848: remove recursion from TracebackException Jan 8, 2021
@iritkatriel iritkatriel marked this pull request as draft January 9, 2021 23:46
@iritkatriel iritkatriel marked this pull request as ready for review January 11, 2021 15:15
gvanrossum
gvanrossum reviewed Jan 11, 2021
View reviewed changes
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.

Awesome! Here are some suggestions.

Lib/traceback.py Show resolved Hide resolved
Lib/traceback.py Outdated Show resolved Hide resolved
Lib/traceback.py Outdated
@@ -481,39 +481,10 @@ def __init__(self, exc_type, exc_value, exc_traceback, *, limit=None,
# permit backwards compat with the existing API, otherwise we
# need stub thunk objects just to glue it together.
# Handle loops in __cause__ or __context__.
_is_chained = _seen is not None
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't need to have a _ prefix, it's just a local variable.

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. Before I realized I can use _seen this was another private arg, so had the _. I'll change to is_recursive_call so it's clear what it's for.

Lib/traceback.py Outdated Show resolved Hide resolved
Lib/traceback.py Outdated
yield from self.stack.format()
yield from self.format_exception_only()

stack = []
Copy link
Member

Choose a reason for hiding this comment

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

It's a bit confusing that we have a local variable stack now but also an attribute exc.stack (completely unrelated).

Lib/traceback.py Outdated
Comment on lines 654 to 655
while stack:
msg, exc = stack.pop()
Copy link
Member

Choose a reason for hiding this comment

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

A faster way might be to use for msg, exc in reversed(stack): (this may also give you inspiration for what to name the variable. :-)

gvanrossum
gvanrossum reviewed Jan 12, 2021
View reviewed changes
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.

LGTM. Now it just needs a news entry. And why is the Ubuntu test failing?

@iritkatriel
Copy link
Member Author

The test failure doesn't look related, it's a tkinter widget test:

cpython/Lib/tkinter/test/test_ttk/test_widgets.py

Line 65 in c1ae21c

def test_identify(self):

@iritkatriel
Copy link
Member Author

Blurbit bot is not working today. Who fixes that?

@gvanrossum
Copy link
Member

Blurbit bot is not working today. Who fixes that?

@Mariatta

But did you know you can just run the blurb tool locally? https://pypi.org/project/blurb/

@gvanrossum
Copy link
Member

The test failure doesn't look related, it's a tkinter widget test:

But possibly a merge from latest master might help (IIRC Serhiy has been working on tkinter recently).

@Mariatta
Copy link
Member

Thanks for raising the issue with blurb-it to my attention. This should be fixed now, please try it again.
In the future, you can also file the issue at https://github.com/python/blurb_it

@iritkatriel
Copy link
Member Author

But possibly a merge from latest master might help (IIRC Serhiy has been working on tkinter recently).

Yes, all the bots are happy now.

Thanks @gvanrossum and @Mariatta.

@gvanrossum gvanrossum merged commit 6dfd173 into python:master Jan 12, 2021
@gvanrossum
Copy link
Member

Thanks Irit!

@iritkatriel iritkatriel deleted the bpo-42848 branch January 13, 2021 09:51
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
@iritkatriel @adorilson
bpo-42848: remove recursion from TracebackException (pythonGH-24158)
5649d06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gvanrossum gvanrossum gvanrossum left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@iritkatriel @gvanrossum @Mariatta @the-knights-who-say-ni @bedevere-bot