Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(762)

#26823: Shrink recursive tracebacks

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 6 months ago by barry
Modified:
1 year, 2 months ago
Reviewers:
ncoghlan
CC:
Georg, Nick Coghlan, stoneleaf, devnull_psf.upfronthosting.co.za, Rosuav, berkerpeksag, storchaka, xiang.zhang, ebarry, Decorater
Visibility:
Public.

Patch Set 1 #

Patch Set 2 #

Patch Set 3 #

Patch Set 4 #

Patch Set 5 #

Patch Set 6 #

Total comments: 4

Patch Set 7 #

Patch Set 8 #

Patch Set 9 #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Doc/library/traceback.rst View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -2 lines 0 comments Download
Doc/whatsnew/3.6.rst View 1 2 3 4 5 6 7 8 1 chunk +4 lines, -1 line 0 comments Download

Messages

Total messages: 1
Nick Coghlan
1 year, 3 months ago #1
Thanks Emanuel, this mostly looks good to me!

My main substantive comment is that the approach of hardcoding the recursion
count won't work cross platform, as the default recursion limit is set
differently depending on how the platform's C runtime behaves. Fortunately,
sys.getrecursionlimit() and len(inspect.stack()) should make it possible to
tweak the tests to avoid the hardcoded assumption.

http://bugs.python.org/review/26823/diff/17947/Lib/test/test_traceback.py
File Lib/test/test_traceback.py (right):

http://bugs.python.org/review/26823/diff/17947/Lib/test/test_traceback.py#new...
Lib/test/test_traceback.py:345: )
Since this is a CPython-only 3.6+ only test case, you could replace these
formatted constants with multi-line f-strings.

http://bugs.python.org/review/26823/diff/17947/Lib/test/test_traceback.py#new...
Lib/test/test_traceback.py:347: result_f = result_f.format(967)
Rather than hardcoding this, you should be able to calculate it dynamically
using `sys.getrecursionlimit()` and `len(inspect.stack())`

That will also cover cross-platform compatibility for platforms where the
default recursion limit is something other than 1000 (some platforms use a
smaller default stack size, so the default recursion limit is correspondingly
smaller).

http://bugs.python.org/review/26823/diff/17947/Lib/traceback.py
File Lib/traceback.py (right):

http://bugs.python.org/review/26823/diff/17947/Lib/traceback.py#newcode393
Lib/traceback.py:393: count = 0
Note for future reference: I considered suggesting an approach based on
"last_entry = (None, None, None)" and building a suitable tuple on each
iteration, but while that would likely make the code shorter, it would also mean
building an extra temporary object on each iteration.

Since you've already written the code this way and it works, there's no sense in
refactoring to something inevitably slower, even if it gives shorter code.

http://bugs.python.org/review/26823/diff/17947/Lib/traceback.py#newcode418
Lib/traceback.py:418: result.append('  [Previous line repeated {} more
times]\n'.format(count-3))
I'd suggest either refactoring to use f-strings, or else pulling out
"format_repeat_count = ' [Previous line repeated {} more times]\n'.format" as a
predefined local.
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7