Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possible implementation of negative limit for traceback functions #66809

Closed
vaultah mannequin opened this issue Oct 12, 2014 · 35 comments
Closed

Possible implementation of negative limit for traceback functions #66809

vaultah mannequin opened this issue Oct 12, 2014 · 35 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@vaultah
Copy link
Mannequin

vaultah mannequin commented Oct 12, 2014

BPO 22619
Nosy @terryjreedy, @rbtcollins, @bitdancer, @serhiy-storchaka, @vaultah
Files
  • 9f618a0c2880.diff
  • 9cb7aaad1d85.diff
  • traceback_rev.diff
  • traceback_rev_fixed.diff
  • traceback_rev2.diff
  • traceback_rev3.diff
  • traceback_negative_limit_2.patch
  • traceback_negative_limit_3.patch
  • traceback_limit_doc.diff
  • traceback_limit_doc_rev2.diff
  • traceback_limit_doc_rev3.diff
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/serhiy-storchaka'
    closed_at = <Date 2015-05-03.10:47:40.298>
    created_at = <Date 2014-10-12.15:24:10.881>
    labels = ['type-feature', 'library']
    title = 'Possible implementation of negative limit for traceback functions'
    updated_at = <Date 2015-05-03.10:47:40.297>
    user = 'https://github.com/vaultah'

    bugs.python.org fields:

    activity = <Date 2015-05-03.10:47:40.297>
    actor = 'serhiy.storchaka'
    assignee = 'serhiy.storchaka'
    closed = True
    closed_date = <Date 2015-05-03.10:47:40.298>
    closer = 'serhiy.storchaka'
    components = ['Library (Lib)']
    creation = <Date 2014-10-12.15:24:10.881>
    creator = 'vaultah'
    dependencies = []
    files = ['36891', '36899', '37600', '37615', '37628', '37632', '37731', '38622', '39152', '39162', '39166']
    hgrepos = []
    issue_num = 22619
    keywords = ['patch']
    message_count = 35.0
    messages = ['229167', '229168', '229173', '229175', '229180', '229183', '229247', '229312', '229887', '229888', '229895', '230660', '231047', '231902', '231975', '233294', '233450', '233541', '233551', '233575', '233592', '233597', '234141', '236453', '238780', '238785', '238800', '241559', '241565', '241684', '241731', '241739', '242092', '242459', '242460']
    nosy_count = 7.0
    nosy_names = ['terry.reedy', 'rbcollins', 'r.david.murray', 'python-dev', 'serhiy.storchaka', 'vaultah', 'flwaultah']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue22619'
    versions = ['Python 3.5']

    @vaultah
    Copy link
    Mannequin Author

    vaultah mannequin commented Oct 12, 2014

    This is the possible patch for this proposal: https://mail.python.org/pipermail/python-ideas/2014-October/029826.html.

    @vaultah vaultah mannequin added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Oct 12, 2014
    @vaultah
    Copy link
    Mannequin Author

    vaultah mannequin commented Oct 12, 2014

    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

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Oct 12, 2014

    You forgot a patch.

    @vaultah
    Copy link
    Mannequin Author

    vaultah mannequin commented Oct 12, 2014

    Sorry, first time posting here; accidently pressed Enter.

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Oct 12, 2014

    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.

    @vaultah
    Copy link
    Mannequin Author

    vaultah mannequin commented Oct 12, 2014

    Thanks. I replied to your comments at Retvield. I'll update the patch tomorrow.

    @vaultah
    Copy link
    Mannequin Author

    vaultah mannequin commented Oct 13, 2014

    Here's the updated (optimized) patch

    @vaultah
    Copy link
    Mannequin Author

    vaultah mannequin commented Oct 14, 2014

    Renamed the cond variable, added tuple unpacking instead of using a single variable.

    @vaultah
    Copy link
    Mannequin Author

    vaultah mannequin commented Oct 23, 2014

    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.

    @bitdancer
    Copy link
    Member

    bitdancer commented Oct 23, 2014

    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'?

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Oct 23, 2014

    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.

    @vaultah
    Copy link
    Mannequin Author

    vaultah mannequin commented Nov 5, 2014

    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.

    @vaultah
    Copy link
    Mannequin Author

    vaultah mannequin commented Nov 11, 2014

    I updated the patch.

    @vaultah
    Copy link
    Mannequin Author

    vaultah mannequin commented Nov 30, 2014

    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.

    @vaultah vaultah mannequin closed this as completed Nov 30, 2014
    @vaultah vaultah mannequin reopened this Nov 30, 2014
    @vaultah vaultah mannequin closed this as completed Dec 1, 2014
    @vaultah
    Copy link
    Mannequin Author

    vaultah mannequin commented Dec 2, 2014

    Moved the latest patch with implementation and tests from bpo-22974 (http://bugs.python.org/issue22974).

    @vaultah vaultah mannequin reopened this Dec 2, 2014
    @vaultah
    Copy link
    Mannequin Author

    vaultah mannequin commented Jan 1, 2015

    Python Developer's Guide said to ping the issue, in case of one-month long inactivity period.

    @vaultah
    Copy link
    Mannequin Author

    vaultah mannequin commented Jan 5, 2015

    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)?

    @vaultah
    Copy link
    Mannequin Author

    vaultah mannequin commented Jan 6, 2015

    Some of the patches (including the latest one) were missing Mercurial header. I'm uploading the properly formatted patch (traceback_rev_fixed.diff)

    @serhiy-storchaka serhiy-storchaka self-assigned this Jan 6, 2015
    @terryjreedy
    Copy link
    Member

    terryjreedy commented Jan 6, 2015

    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?

    @vaultah
    Copy link
    Mannequin Author

    vaultah mannequin commented Jan 7, 2015

    Thank you, Terry. You got the proposal right. I'm glad you noticed the issues with tests, I updated the patch to fix them.

    @vaultah
    Copy link
    Mannequin Author

    vaultah mannequin commented Jan 7, 2015

    Hold on, I found 2 more bugs. Will update the patch soon.

    @vaultah
    Copy link
    Mannequin Author

    vaultah mannequin commented Jan 7, 2015

    I improved tests for *_stack and *_tb functions, fixed a few typos and added more comments.

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Jan 16, 2015

    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.

    @vaultah
    Copy link
    Mannequin Author

    vaultah mannequin commented Feb 23, 2015

    Ping

    @vaultah
    Copy link
    Mannequin Author

    vaultah mannequin commented Mar 21, 2015

    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?

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Mar 21, 2015

    I proposed changed patch. It needs a review.

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Mar 21, 2015

    Due to changes in bpo-17911 the patch no longer applied cleanly an should be rewritten. But changes in bpo-17911 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.

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Apr 19, 2015

    Needed a documentation. I'm not interested in writing it.

    @vaultah
    Copy link
    Mannequin Author

    vaultah mannequin commented Apr 19, 2015

    I'll do that tomorrow. The patch still needs a review though...

    @vaultah
    Copy link
    Mannequin Author

    vaultah mannequin commented Apr 20, 2015

    Here's the documentation patch.

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Apr 21, 2015

    As francismb noted, there are too long lines in the patch. They should be wrapped.

    @vaultah
    Copy link
    Mannequin Author

    vaultah mannequin commented Apr 21, 2015

    Thanks, completely missed the abs(limit) part. Here's the updated documentation patch.

    @rbtcollins
    Copy link
    Member

    rbtcollins commented Apr 27, 2015

    Nice, looks pretty elegant to me. I have nothing to add to the prior reviewers comments.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 3, 2015

    New changeset eb6052605fd8 by Serhiy Storchaka in branch 'default':
    Issue bpo-22619: Added negative limit support in the traceback module.
    https://hg.python.org/cpython/rev/eb6052605fd8

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented May 3, 2015

    Thank you for your contribution Dmitry.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants