classification
Title: traceback.py has a lot of code duplication
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: benjamin.peterson, isoschiz, python-dev, r.david.murray, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2013-04-06 17:23 by isoschiz, last changed 2013-04-29 20:11 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
traceback.diff isoschiz, 2013-04-06 17:23 Refactor of traceback.py review
issue17646-2.diff isoschiz, 2013-04-18 23:22 Refactor of traceback.py
issue17646-3.diff isoschiz, 2013-04-19 20:54 Refactor of traceback.py
issue17646-4.diff isoschiz, 2013-04-20 00:10 Refactor of traceback.py review
issue17646-5.diff isoschiz, 2013-04-22 11:02 Refactor of traceback.py; now with added tests review
Messages (18)
msg186142 - (view) Author: Martin Morrison (isoschiz) * Date: 2013-04-06 17:23
traceback.py contains a lot of code duplication, which makes it fragile in the face of changes (i.e. special cases) to the stack/traceback output (I am separately working on just such a change).

The attached patch refactors the code to reduce to a single function for each bit of logic, wrapped by the various existing APIs. The new helper functions are refactored as generators so as not to create unnecessary transient lists (not that stacks usually get very big anyway).

I've fully tested the replacement module, and it passes all the traceback tests in the standard suite.
msg186152 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2013-04-06 19:27
How much does this overlap with #17491?
msg186166 - (view) Author: Martin Morrison (isoschiz) * Date: 2013-04-06 20:56
I hadn't spotted that one!

My refactor goes further (consolidates all duplicates, not just the tb ones), is implemented the way you suggested therein (print in terms of extract), and is "more efficient" in that it uses generators for all intermediate iterators. 

However, I'm happy to either attach my diff to that issue, refactor mine in terms of that one, or whatever you think is best.
msg187135 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2013-04-17 03:46
The other patch author hasn't updated his patch, so this issue can just superseded the other one.

I uploaded a few review comments.
msg187252 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2013-04-18 14:01
Okay. Will you upload the latest version?
msg187300 - (view) Author: Martin Morrison (isoschiz) * Date: 2013-04-18 22:13
Latest upload has all of the agreed markups from the review.
msg187309 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2013-04-18 23:13
What's this "f_invisible" thing?
msg187310 - (view) Author: Martin Morrison (isoschiz) * Date: 2013-04-18 23:22
Sorry, that is an unrelated change I was experimenting with, which I accidentally forgot to take out.

Please see the new uploaded diff.
msg187325 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2013-04-19 00:51
patching file Lib/traceback.py
patch: **** malformed patch at line 94: @@ -50,25 +90,7 @@
msg187385 - (view) Author: Martin Morrison (isoschiz) * Date: 2013-04-19 20:54
How embarrassing - I guess that'll teach me to try to doctor the diff file. :-)

I've now regenerated the diff, and re-run all the testing, etc. Apologies for the churn.
msg187390 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2013-04-19 22:56
Also, this patch seems to break test_zipimport.
msg187391 - (view) Author: Martin Morrison (isoschiz) * Date: 2013-04-20 00:10
print_tb() wasn't passing the file argument through to print_list() - now fixed. I also verified none of the other functions had a similar bug.
msg187402 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2013-04-20 02:54
It would be great to have a test for that. :)
msg187429 - (view) Author: Martin Morrison (isoschiz) * Date: 2013-04-20 14:28
On 20/04/2013 03:54, Benjamin Peterson wrote:
> It would be great to have a test for that. :)

I was afraid you'd say that. ;-)

I'll look at adding test cases to cover the functions not currently
covered (seems most of the print functions aren't, and all of the
'stack' functions aren't).
msg187445 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-04-20 17:55
Could print_exception() in Lib/idlelib/run.py reuse new traceback functions?
msg187462 - (view) Author: Martin Morrison (isoschiz) * Date: 2013-04-20 21:18
On 20 Apr 2013, at 18:55, Serhiy Storchaka <report@bugs.python.org> wrote:
> Serhiy Storchaka added the comment:
> 
> Could print_exception() in Lib/idlelib/run.py reuse new traceback functions?

Actually, cleaning up code like that in Idle and the code module and import.c was what I was aiming to solve when I stumbled upon the duplication in traceback.py - once this patch is in, I plan to raise a separate issue with my suggestion for that clean up.

To answer your question directly, the changes I've made here don't solve the problem, no. More invasive changes are required, which is why I raised this cleanup portion of my change separately (on the assumption this change alone wouldn't be controversial, but my wider change might be).
msg187553 - (view) Author: Martin Morrison (isoschiz) * Date: 2013-04-22 11:02
Attached another diff, which includes new test cases to cover all functions that weren't covered before - expect print_last. I couldn't see a way of getting sys.last_* set to test that function (it seems to require an "unhandled exception"). However, the function is a simple wrapper around print_exception, and I have tested it manually in the interactive interpreter.
msg188093 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2013-04-29 20:11
New changeset 84cef4f1999a by Benjamin Peterson in branch 'default':
refactor traceback.py to reduce code duplication (closes #17646)
http://hg.python.org/cpython/rev/84cef4f1999a
History
Date User Action Args
2013-04-29 20:11:24python-devsetstatus: open -> closed

nosy: + python-dev
messages: + msg188093

resolution: fixed
stage: resolved
2013-04-22 11:02:21isoschizsetfiles: + issue17646-5.diff

messages: + msg187553
2013-04-20 21:18:08isoschizsetmessages: + msg187462
2013-04-20 17:55:41serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg187445
2013-04-20 14:28:34isoschizsetmessages: + msg187429
2013-04-20 02:54:38benjamin.petersonsetmessages: + msg187402
2013-04-20 00:10:00isoschizsetfiles: + issue17646-4.diff

messages: + msg187391
2013-04-19 22:56:01benjamin.petersonsetmessages: + msg187390
2013-04-19 20:54:58isoschizsetfiles: + issue17646-3.diff

messages: + msg187385
2013-04-19 00:51:25benjamin.petersonsetmessages: + msg187325
2013-04-18 23:22:32isoschizsetfiles: + issue17646-2.diff

messages: + msg187310
2013-04-18 23:20:46isoschizsetfiles: - issue17646-2.diff
2013-04-18 23:13:43benjamin.petersonsetmessages: + msg187309
2013-04-18 22:13:07isoschizsetfiles: + issue17646-2.diff

messages: + msg187300
2013-04-18 14:01:29benjamin.petersonsetmessages: + msg187252
2013-04-17 03:46:12benjamin.petersonlinkissue17491 superseder
2013-04-17 03:46:02benjamin.petersonsetmessages: + msg187135
2013-04-06 20:56:24isoschizsetmessages: + msg186166
2013-04-06 19:27:42benjamin.petersonsetnosy: + benjamin.peterson
messages: + msg186152
2013-04-06 18:14:30r.david.murraysetnosy: + r.david.murray
2013-04-06 17:23:44isoschizcreate