Skip to content

bpo-42848: remove recursion from TracebackException#24158

Merged
gvanrossum merged 7 commits into
python:masterfrom
iritkatriel:bpo-42848
Jan 12, 2021
Merged

bpo-42848: remove recursion from TracebackException#24158
gvanrossum merged 7 commits into
python:masterfrom
iritkatriel:bpo-42848

Conversation

@iritkatriel

@iritkatriel iritkatriel commented Jan 8, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

@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

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

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.

Comment thread Lib/traceback.py Outdated
Comment thread Lib/traceback.py Outdated
Comment thread Lib/traceback.py Outdated

@gvanrossum gvanrossum Jan 11, 2021

Copy link
Copy Markdown
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.

@iritkatriel iritkatriel Jan 12, 2021

Copy link
Copy Markdown
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.

Comment thread Lib/traceback.py Outdated
Comment thread Lib/traceback.py Outdated

@gvanrossum gvanrossum Jan 11, 2021

Copy link
Copy Markdown
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).

Comment thread Lib/traceback.py Outdated
Comment on lines 654 to 655

@gvanrossum gvanrossum Jan 11, 2021

Copy link
Copy Markdown
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

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

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

iritkatriel commented Jan 12, 2021

Copy link
Copy Markdown
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

iritkatriel commented Jan 12, 2021

Copy link
Copy Markdown
Member Author

Blurbit bot is not working today. Who fixes that?

@gvanrossum

gvanrossum commented Jan 12, 2021

Copy link
Copy Markdown
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

gvanrossum commented Jan 12, 2021

Copy link
Copy Markdown
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

Mariatta commented Jan 12, 2021

Copy link
Copy Markdown
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

iritkatriel commented Jan 12, 2021

Copy link
Copy Markdown
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

gvanrossum commented Jan 12, 2021

Copy link
Copy Markdown
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