classification
Title: Possible implementation of negative limit for traceback functions
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: flwaultah, python-dev, r.david.murray, rbcollins, serhiy.storchaka, terry.reedy, vaultah
Priority: normal Keywords: patch

Created on 2014-10-12 15:24 by vaultah, last changed 2015-05-03 10:47 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
9f618a0c2880.diff vaultah, 2014-10-12 16:40 review
9cb7aaad1d85.diff vaultah, 2014-10-13 14:46 review
traceback_rev.diff vaultah, 2015-01-05 09:41
traceback_rev_fixed.diff vaultah, 2015-01-06 13:24 review
traceback_rev2.diff vaultah, 2015-01-07 10:57 review
traceback_rev3.diff vaultah, 2015-01-07 18:37 review
traceback_negative_limit_2.patch serhiy.storchaka, 2015-01-16 18:35 review
traceback_negative_limit_3.patch serhiy.storchaka, 2015-03-21 13:43 review
traceback_limit_doc.diff vaultah, 2015-04-20 20:46 review
traceback_limit_doc_rev2.diff vaultah, 2015-04-21 20:29 review
traceback_limit_doc_rev3.diff vaultah, 2015-04-22 11:25 review
Messages (35)
msg229167 - (view) Author: Dmitry Kazakov (vaultah) * Date: 2014-10-12 15:24
This is the possible patch for this proposal: https://mail.python.org/pipermail/python-ideas/2014-October/029826.html.
msg229168 - (view) Author: Dmitry Kazakov (vaultah) * Date: 2014-10-12 15:24
def _extract_tb_or_stack_iter(curr, limit, extractor):
    # Distinguish frames from tracebacks (need to import types)
    if limit is None:
        limit = getattr(sys, 'tracebacklimit', None)
    elif limit < 0 and isinstance(curr, types.TracebackType):
        seq = list(_extract_tb_or_stack_iter(curr, None, extractor))
        yield from seq[limit:]
    else:
        pass
msg229173 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-10-12 16:19
You forgot a patch.
msg229175 - (view) Author: Dmitry Kazakov (vaultah) * Date: 2014-10-12 16:39
Sorry, first time posting here; accidently pressed Enter.
msg229180 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-10-12 17:17
I have added comments on Retvield (a system for patch reviewing). You should receive an email about this.

In general the patch is not optimal. There is no need to load source lines for dropped entries.
msg229183 - (view) Author: Dmitry Kazakov (vaultah) * Date: 2014-10-12 18:14
Thanks. I replied to your comments at Retvield. I'll update the patch tomorrow.
msg229247 - (view) Author: Dmitry Kazakov (vaultah) * Date: 2014-10-13 14:46
Here's the updated (optimized) patch
msg229312 - (view) Author: Dmitry Kazakov (vaultah) * Date: 2014-10-14 16:39
Renamed the cond variable, added tuple unpacking instead of using a single variable.
msg229887 - (view) Author: Dmitry Kazakov (vaultah) * Date: 2014-10-23 18:17
I think the reason this patch hasn't been discussed well is that it only changes the behavior for traceback.*_tb functions that only deal with tracebacks. I commented on the review page that we don't have to change the behavior of traceback.*_stack functions to make it obvious. Let me show an example:

import sys

def a():
    b()

def b():
    c()

def c():
    d()

def d():
    def e():
        print_stack(limit=2) # Last 2 entries
        ''' Output:
            File "file", line 331, in d                                                                                                                                                                                       
              e()                                                                                                                                                                                                                         
            File "file", line 328, in e
              print_stack(limit=2) # Last 2 entries '''
        raise Exception
    e()
    
try:
    a()
except Exception:
    print_exc(limit=2) # 2 entries from the caller
    ''' Output:
        Traceback (most recent call last):
          File "file", line 336, in <module>
            a()
          File "file", line 318, in a
            b()
        Exception '''

If we decide to unify the behavior of *_tb and *_stack functions, the change will break some existing code, although the breakage will be purely cosmetic.
msg229888 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-10-23 18:40
More likely the lack of discussion is just that Serhiy is busy :)

Breaking code is to be avoided if possible.  Can you give an example of the "cosmetic" change?

I haven't fully reviewed the patch, but a more meaningful name than 'condition' might make the code more readable.  Perhaps 'handling_negative_limit'?
msg229895 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-10-23 19:37
Last patch is more complicated and needs more time to review.

I suppose the code would be more clear if split _extract_tb_or_stack_iter() to parts. One generator just generates (filename, lineno, name, f.f_globals) tuples. Then these tuples either handled directly or passed through a deque with fixed size.

And some tests would be good.
msg230660 - (view) Author: Dmitry Kazakov (vaultah) * Date: 2014-11-05 06:13
I'm not sure what would be the best way to support negative limit for stack functions. Actually, I think the current behavior is not intuitive. For example we could make positive limit mean the same thing for traceback and stack functions (load N entries, starting from the caller), but in that case the change will inverse the behavior of code that uses stack functions. Since the traceback module is mostly used for printing/formatting the difference won't be crucial.
msg231047 - (view) Author: Dmitry Kazakov (vaultah) * Date: 2014-11-11 19:56
I updated the patch.
msg231902 - (view) Author: Dmitry Kazakov (vaultah) * Date: 2014-11-30 19:50
Revisiting this issue, I realize that I made quite a few mistakes (because this was the first issue I submitted). The patch is definitely minor, and I'm no longer interested in it. This issue may now be closed. Cheers.
msg231975 - (view) Author: Dmitry Kazakov (vaultah) * Date: 2014-12-02 04:52
Moved the latest patch with implementation and tests from issue 22974 (http://bugs.python.org/issue22974).
msg233294 - (view) Author: Dmitry Kazakov (vaultah) * Date: 2015-01-01 19:48
Python Developer's Guide said to ping the issue, in case of one-month long inactivity period.
msg233450 - (view) Author: Dmitry Kazakov (vaultah) * Date: 2015-01-05 09:41
I understand that this issue is far from being important, but this is going to be the fourth unreviewed file in this issue. I noted all your comments to me and fixed the patch accordingly, but ever since November I'm the only one who posts something here. I pinged the issue about 4 days ago (after a month of utter silence) and still didn't get any response. Could someone review the latest patch (the one I attach to this message - traceback_rev.diff) or at least say what's wrong with it (or with this issue, or with something else)?
msg233541 - (view) Author: Dmitry Kazakov (vaultah) * Date: 2015-01-06 13:24
Some of the patches (including the latest one) were missing Mercurial header. I'm uploading the properly formatted patch (traceback_rev_fixed.diff)
msg233551 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2015-01-06 21:07
For future reference, a better initial post would have been more self-contained and explanatory, something like the following.  (If I got the proposal wrong, all the more reason to explain it clearly).
---
Several functions in the traceback module take a *limit* argument to limit output to the first n 'stack trace entries'.  This issue proposes that negative limits should instead limit output to the last n entries. This idea was discussed on python ideas and approved by Guido there.
https://mail.python.org/pipermail/python-ideas/2014-October/029826.html.
---

Do the new tests pass with n=0?  If not, do the functions have a sensible behavior (print header and no entries, rather than raising) that could be tested?
msg233575 - (view) Author: Dmitry Kazakov (vaultah) * Date: 2015-01-07 10:57
Thank you, Terry. You got the proposal right. I'm glad you noticed the issues with tests, I updated the patch to fix them.
msg233592 - (view) Author: Dmitry Kazakov (vaultah) * Date: 2015-01-07 18:22
Hold on, I found 2 more bugs. Will update the patch soon.
msg233597 - (view) Author: Dmitry Kazakov (vaultah) * Date: 2015-01-07 18:37
I improved tests for *_stack and *_tb functions, fixed a few typos and added more comments.
msg234141 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-01-16 18:35
Thank you for your patches Dmitry.

But I think that the code can be made simpler. Here is a patch which refactors extracting code. It splits the code on few generators which do different tasks: iterate over tracebacks or frames linked list, limit the size of proceeded sequence, and generates required fields. Note that the behavior of extract_stack() with negative limit differs if sys.tracebacklimit is specified and less than the length of full traceback. Tests are changed too, now they test all combinations of the limit parameter and sys.tracebacklimit.
msg236453 - (view) Author: Dmitry Kazakov (vaultah) * Date: 2015-02-23 17:59
Ping
msg238780 - (view) Author: Dmitry Kazakov (vaultah) * Date: 2015-03-21 09:32
Again, I'm honestly sorry if I'm being annoying, but is there anything else that needs to be done in order to make this issue "resolved"? The stage is set to "patch review", although there were no messages posted since the latest patch was submitted and there're no comments to the Patch set 6 in Rietveld.

> If your patch has not received any notice from reviewers (i.e., no comment made) after one month, first “ping” the issue on the issue tracker to remind the nosy list that the patch needs a review. 

Is this true for the "patch review" stage? I pinged this issue about a month ago, but haven't got any response; should I have emailed python-dev@python.org even if the patch is not mine?
msg238785 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-21 10:56
I proposed changed patch. It needs a review.
msg238800 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-21 13:43
Due to changes in issue17911 the patch no longer applied cleanly an should be rewritten. But changes in issue17911 are close to my patch. _tb_frame_lineno_iter matches walk_tb and _stack_frame_lineno_iter matches walk_stack. So new patch is simpler.
msg241559 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-04-19 19:55
Needed a documentation. I'm not interested in writing it.
msg241565 - (view) Author: Dmitry Kazakov (vaultah) * Date: 2015-04-19 21:06
I'll do that tomorrow. The patch still needs a review though...
msg241684 - (view) Author: Dmitry Kazakov (vaultah) * Date: 2015-04-20 20:46
Here's the documentation patch.
msg241731 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-04-21 18:33
As francismb noted, there are too long lines in the patch. They should be wrapped.
msg241739 - (view) Author: Dmitry Kazakov (vaultah) * Date: 2015-04-21 20:00
Thanks, completely missed the abs(limit) part. Here's the updated documentation patch.
msg242092 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-04-27 01:33
Nice, looks pretty elegant to me. I have nothing to add to the prior reviewers comments.
msg242459 - (view) Author: Roundup Robot (python-dev) Date: 2015-05-03 10:32
New changeset eb6052605fd8 by Serhiy Storchaka in branch 'default':
Issue #22619: Added negative limit support in the traceback module.
https://hg.python.org/cpython/rev/eb6052605fd8
msg242460 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-05-03 10:47
Thank you for your contribution Dmitry.
History
Date User Action Args
2015-05-03 10:47:40serhiy.storchakasetstatus: open -> closed
resolution: fixed
messages: + msg242460

stage: patch review -> resolved
2015-05-03 10:32:22python-devsetnosy: + python-dev
messages: + msg242459
2015-04-27 01:33:27rbcollinssetmessages: + msg242092
2015-04-22 11:25:23vaultahsetfiles: + traceback_limit_doc_rev3.diff
2015-04-21 20:29:47vaultahsetfiles: + traceback_limit_doc_rev2.diff
2015-04-21 20:14:20vaultahsetfiles: - traceback_limit_doc_rev.diff
2015-04-21 20:00:35vaultahsetfiles: + traceback_limit_doc_rev.diff

messages: + msg241739
2015-04-21 18:33:32serhiy.storchakasetmessages: + msg241731
2015-04-20 20:46:20vaultahsetfiles: + traceback_limit_doc.diff

messages: + msg241684
2015-04-19 21:06:52vaultahsetmessages: + msg241565
2015-04-19 19:55:36serhiy.storchakasetmessages: + msg241559
2015-04-19 19:06:09flwaultahsetnosy: + flwaultah
2015-03-21 13:43:42serhiy.storchakasetfiles: + traceback_negative_limit_3.patch
nosy: + rbcollins
messages: + msg238800

2015-03-21 10:56:20serhiy.storchakasetmessages: + msg238785
2015-03-21 09:32:31vaultahsetmessages: + msg238780
2015-02-23 17:59:03vaultahsetmessages: + msg236453
2015-01-16 18:35:59serhiy.storchakasetfiles: + traceback_negative_limit_2.patch

messages: + msg234141
stage: patch review
2015-01-07 20:30:12vaultahsetfiles: - traceback_patch_2.diff
2015-01-07 20:30:05vaultahsetfiles: - traceback.diff
2015-01-07 20:29:38vaultahsetfiles: - tb_patch_2.diff
2015-01-07 18:37:48vaultahsetfiles: + traceback_rev3.diff

messages: + msg233597
2015-01-07 18:22:18vaultahsetmessages: + msg233592
2015-01-07 10:57:12vaultahsetfiles: + traceback_rev2.diff

messages: + msg233575
2015-01-06 21:07:52terry.reedysetnosy: + terry.reedy
messages: + msg233551
2015-01-06 15:51:13serhiy.storchakasetassignee: serhiy.storchaka
2015-01-06 13:24:35vaultahsetfiles: + traceback_rev_fixed.diff

messages: + msg233541
2015-01-05 09:41:51vaultahsetfiles: + traceback_rev.diff

messages: + msg233450
2015-01-01 19:48:15vaultahsetmessages: + msg233294
2014-12-02 04:55:59berker.peksaglinkissue22974 superseder
2014-12-02 04:52:49vaultahsetstatus: closed -> open
files: + traceback_patch_2.diff
messages: + msg231975
2014-12-01 16:00:16vaultahsetstatus: open -> closed
2014-11-30 20:35:42vaultahsetstatus: closed -> open
2014-11-30 20:21:52vaultahsetstatus: open -> closed
2014-11-30 19:50:39vaultahsetmessages: + msg231902
2014-11-11 19:56:51vaultahsetfiles: + traceback.diff

messages: + msg231047
2014-11-05 06:13:51vaultahsetmessages: + msg230660
2014-10-23 19:37:34serhiy.storchakasetmessages: + msg229895
2014-10-23 18:40:33r.david.murraysetnosy: + r.david.murray
messages: + msg229888
2014-10-23 18:17:10vaultahsetmessages: + msg229887
2014-10-14 17:51:08vaultahsetfiles: + tb_patch_2.diff
2014-10-14 17:50:28vaultahsethgrepos: - hgrepo278
2014-10-14 17:50:11vaultahsetfiles: - 4a0ec19e4288.diff
2014-10-14 17:49:28vaultahsetfiles: + 4a0ec19e4288.diff
2014-10-14 17:48:51vaultahsethgrepos: + hgrepo278
2014-10-14 17:46:19vaultahsetfiles: - tb_patch.diff
2014-10-14 16:39:12vaultahsetfiles: + tb_patch.diff

messages: + msg229312
2014-10-13 17:19:41vaultahsethgrepos: - hgrepo275
2014-10-13 15:04:01vaultahsethgrepos: - hgrepo277
2014-10-13 14:46:51vaultahsetfiles: + 9cb7aaad1d85.diff
2014-10-13 14:46:06vaultahsethgrepos: + hgrepo277
messages: + msg229247
2014-10-12 18:14:52vaultahsetmessages: + msg229183
2014-10-12 17:17:16serhiy.storchakasetmessages: + msg229180
2014-10-12 16:40:09vaultahsetfiles: + 9f618a0c2880.diff
keywords: + patch
2014-10-12 16:39:36vaultahsethgrepos: + hgrepo275
messages: + msg229175
2014-10-12 16:19:50serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg229173
2014-10-12 15:24:56vaultahsetmessages: + msg229168
2014-10-12 15:24:10vaultahcreate