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

#17911: Extracting tracebacks does too much work

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years, 7 months ago by guido
Modified:
4 years, 12 months ago
Reviewers:
jimjjewett, ncoghlan, pitrou, robertc, berker.peksag
CC:
gvanrossum, rhettinger, terry.reedy, Nick Coghlan, AntoinePitrou, haypo, rbcollins, ned.deily, devnull_psf.upfronthosting.co.za, eric.snow, Martin Panter, mahmoud_hatnote.com, Yury Selivanov, adaptivelogic, martius, xonatius
Visibility:
Public.

Patch Set 1 #

Patch Set 2 #

Total comments: 3

Patch Set 3 #

Patch Set 4 #

Patch Set 5 #

Patch Set 6 #

Total comments: 22

Patch Set 7 #

Patch Set 8 #

Total comments: 16

Patch Set 9 #

Patch Set 10 #

Total comments: 8

Patch Set 11 #

Total comments: 15

Patch Set 12 #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Lib/code.py View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +8 lines, -25 lines 0 comments Download
Lib/_pydecimal.py View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -1 line 0 comments Download
Lib/traceback.py View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +2 lines, -2 lines 0 comments Download

Messages

Total messages: 8
Jim.J.Jewett
http://bugs.python.org/review/17911/diff/8639/Doc/library/traceback.rst File Doc/library/traceback.rst (right): http://bugs.python.org/review/17911/diff/8639/Doc/library/traceback.rst#newcode70 Doc/library/traceback.rst:70: .. function:: extract_exception(type, value, traceback, limit=None, chain=True) This still ...
5 years, 11 months ago #1
Nick Coghlan
The general approach looks good to me. Some concrete concerns regarding possible ambiguity in the ...
5 years, 1 month ago #2
AntoinePitrou
Thanks! Some comments below. http://bugs.python.org/review/17911/diff/13734/Doc/library/linecache.rst File Doc/library/linecache.rst (right): http://bugs.python.org/review/17911/diff/13734/Doc/library/linecache.rst#newcode46 Doc/library/linecache.rst:46: .. function:: deferredcache(filename, module_globals) What ...
5 years ago #3
rbcollins
http://bugs.python.org/review/17911/diff/13734/Doc/library/linecache.rst File Doc/library/linecache.rst (right): http://bugs.python.org/review/17911/diff/13734/Doc/library/linecache.rst#newcode46 Doc/library/linecache.rst:46: .. function:: deferredcache(filename, module_globals) On 2015/02/05 20:08:23, AntoinePitrou wrote: ...
4 years, 12 months ago #4
AntoinePitrou
http://bugs.python.org/review/17911/diff/13836/Lib/traceback.py File Lib/traceback.py (right): http://bugs.python.org/review/17911/diff/13836/Lib/traceback.py#newcode272 Lib/traceback.py:272: f = sys._getframe().f_back.f_back On 2015/03/03 22:46:13, rbcollins wrote: > ...
4 years, 12 months ago #5
AntoinePitrou
http://bugs.python.org/review/17911/diff/14077/Doc/library/traceback.rst File Doc/library/traceback.rst (right): http://bugs.python.org/review/17911/diff/14077/Doc/library/traceback.rst#newcode139 Doc/library/traceback.rst:139: .. function:: walk_stack(f) You have to materialize that f ...
4 years, 12 months ago #6
rbcollins
http://bugs.python.org/review/17911/diff/14077/Doc/library/traceback.rst File Doc/library/traceback.rst (right): http://bugs.python.org/review/17911/diff/14077/Doc/library/traceback.rst#newcode139 Doc/library/traceback.rst:139: .. function:: walk_stack(f) On 2015/03/03 23:33:43, AntoinePitrou wrote: > ...
4 years, 12 months ago #7
berkerpeksag
4 years, 12 months ago #8
http://bugs.python.org/review/17911/diff/14078/Doc/library/traceback.rst
File Doc/library/traceback.rst (right):

http://bugs.python.org/review/17911/diff/14078/Doc/library/traceback.rst#newc...
Doc/library/traceback.rst:159: :class:`.TracebackException` objects are created
from actual exceptions to
Could you define TracebackException by using ``.. exception::`` directive (like
you already did for FrameSummary)? This way you won't need to add versionadded
directives to all methods.

http://bugs.python.org/review/17911/diff/14078/Doc/library/traceback.rst#newc...
Doc/library/traceback.rst:162: .. classmethod:: `.from_exception`(exc,
limit=None, lookup_lines=True)
no need to add `.xxx` if you apply the suggestion above

http://bugs.python.org/review/17911/diff/14078/Doc/library/traceback.rst#newc...
Doc/library/traceback.rst:193: .. method:: 
TracebackException.format(chain=True)
no need to add `TracebackException.` if you apply the suggestion above

http://bugs.python.org/review/17911/diff/14078/Doc/library/traceback.rst#newc...
Doc/library/traceback.rst:197: If chain is not *True*, *__cause__* and
*__context__* will not be formatted.
chain -> *chain*
*True* -> ``True``
*__cause__* and *__context__* -> ``__cause__`` and ``__context__``

We are using * for parameter names. See also
https://docs.python.org/devguide/documenting.html

http://bugs.python.org/review/17911/diff/14078/Lib/linecache.py
File Lib/linecache.py (right):

http://bugs.python.org/review/17911/diff/14078/Lib/linecache.py#newcode110
Lib/linecache.py:110: [line+'\n' for line in data.splitlines()], fullname
or data.splitlines(keepends=True)?

http://bugs.python.org/review/17911/diff/14078/Lib/linecache.py#newcode158
Lib/linecache.py:158: if len(cache[filename]) == 1:
or just ``return len(cache[filename]) == 1``?

http://bugs.python.org/review/17911/diff/14078/Lib/test/test_traceback.py
File Lib/test/test_traceback.py (left):

http://bugs.python.org/review/17911/diff/14078/Lib/test/test_traceback.py#old...
Lib/test/test_traceback.py:480: def test_main():
Since you touched the file, could you also update to use unittest test
discovery? e.g.

if __name__ == '__main__':
    unittest.main()

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

http://bugs.python.org/review/17911/diff/14078/Lib/test/test_traceback.py#new...
Lib/test/test_traceback.py:490: linecache.clearcache()
perhaps this can go into a setUp method?

http://bugs.python.org/review/17911/diff/14078/Lib/test/test_traceback.py#new...
Lib/test/test_traceback.py:496: self.assertEqual(None, f.locals)
assertIsNone

http://bugs.python.org/review/17911/diff/14078/Lib/test/test_traceback.py#new...
Lib/test/test_traceback.py:501: self.assertEqual(None, f._line)
assertIsNone

http://bugs.python.org/review/17911/diff/14078/Lib/test/test_traceback.py#new...
Lib/test/test_traceback.py:578: self.assertEqual(None, exc.__cause__)
assertIsNone

http://bugs.python.org/review/17911/diff/14078/Lib/test/test_traceback.py#new...
Lib/test/test_traceback.py:579: self.assertEqual(None, exc.__context__)
assertIsNone

http://bugs.python.org/review/17911/diff/14078/Lib/test/test_traceback.py#new...
Lib/test/test_traceback.py:580: self.assertEqual(False,
exc.__suppress_context__)
assertFalse

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

http://bugs.python.org/review/17911/diff/14078/Lib/traceback.py#newcode210
Lib/traceback.py:210: class FrameSummary:
Could you also update traceback.__all__?

http://bugs.python.org/review/17911/diff/14078/Lib/traceback.py#newcode300
Lib/traceback.py:300: :param frame_gen: A generator that yields (frame, lineno)
tuples to
If I remember correctly, rst syntax in stdlib code is useless and we don't use
it.
Sign in to reply to this message.

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