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

Expand traceback module API to accept just an exception as an argument #70577

Closed
brettcannon opened this issue Feb 18, 2016 · 19 comments
Closed

Expand traceback module API to accept just an exception as an argument #70577

brettcannon opened this issue Feb 18, 2016 · 19 comments
Labels
3.10 stdlib type-feature

Comments

@brettcannon
Copy link
Member

@brettcannon brettcannon commented Feb 18, 2016

BPO 26389
Nosy @brettcannon, @rhettinger, @terryjreedy, @vadmium, @Carreau, @csabella, @ZackerySpytz, @pablogsal
PRs
  • #327
  • #22610
  • 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 = None
    closed_at = <Date 2020-11-05.22:19:23.268>
    created_at = <Date 2016-02-18.22:59:10.834>
    labels = ['type-feature', 'library', '3.10']
    title = 'Expand traceback module API to accept just an exception as an argument'
    updated_at = <Date 2021-02-12.22:18:58.626>
    user = 'https://github.com/brettcannon'

    bugs.python.org fields:

    activity = <Date 2021-02-12.22:18:58.626>
    actor = 'terry.reedy'
    assignee = 'none'
    closed = True
    closed_date = <Date 2020-11-05.22:19:23.268>
    closer = 'pablogsal'
    components = ['Library (Lib)']
    creation = <Date 2016-02-18.22:59:10.834>
    creator = 'brett.cannon'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 26389
    keywords = ['patch']
    message_count = 19.0
    messages = ['260485', '260554', '260559', '260571', '288615', '288618', '288621', '288645', '288660', '288671', '288673', '288857', '288870', '339681', '339885', '355021', '356574', '356575', '380438']
    nosy_count = 8.0
    nosy_names = ['brett.cannon', 'rhettinger', 'terry.reedy', 'martin.panter', 'mbussonn', 'cheryl.sabella', 'ZackerySpytz', 'pablogsal']
    pr_nums = ['327', '22610']
    priority = 'low'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue26389'
    versions = ['Python 3.10']

    @brettcannon
    Copy link
    Member Author

    @brettcannon brettcannon commented Feb 18, 2016

    When reading https://docs.python.org/3/library/traceback.html#traceback.print_exception I noticed that everything takes a traceback or a set of exception type, instance, and traceback. But since Python 3.0 all of that information is contained in a single exception object. So there's no reason to expand the APIs in the traceback module that take an exception to just take an exception instance and infer the exception type and grab the exception from exception instance itself.

    @brettcannon brettcannon added the stdlib label Feb 18, 2016
    @terryjreedy
    Copy link
    Member

    @terryjreedy terryjreedy commented Feb 20, 2016

    You title and post don't seem to match. The title says to expand the API and the post says there is no reason to expand the API. Did you mean 'good reason'? Also, I think you meant 'grab the traceback' (from the exception) rather than 'grab the exception'.

    How would you get from here to there and change required etype, value, tb to just required value, without breaking old code?

    @vadmium
    Copy link
    Member

    @vadmium vadmium commented Feb 20, 2016

    There is precedent with Python 2’s “raise” statement for accepting an exception instance for the first parameter (where an exception class would otherwise be passed). Also, generator.throw() supports this; see bpo-14911 for clarifying its documentation.

    I would support changing the following signatures so that the first parameter could hold the value, not just the type:

    print_exception(etype, value=None, tb=None, limit=None, ...)
    format_exception_only(etype, value=None)
    format_exception(etype, value=None, tb=None, limit=None, ...)
    TracebackException(exc_type, exc_value=None, exc_traceback=None, *, ...)

    @vadmium vadmium added the type-feature label Feb 20, 2016
    @brettcannon
    Copy link
    Member Author

    @brettcannon brettcannon commented Feb 20, 2016

    So Terry's right that in my haste to write down the idea I contradicted myself. I do want to tweak the APIs in the traceback module to accept only an exception. The question is whether we need entirely new functions or if the pre-existing ones can be updated to work with just an exception.

    And if we were to tweak the existing functions instead of add new ones, I would do it by making all arguments optional and adding a new keyword-only argument called exc that took the exception. Either exc would be set or the old arguments would need to all be set, and it would be an error to set both sets of arguments. And when the old arguments were taken away then all arguments for those functions would become keyword-only.

    @Carreau
    Copy link
    Mannequin

    @Carreau Carreau mannequin commented Feb 27, 2017

    Just came across that with wanting to use print_exception and got the same idea.

    It seem like in print_exception, and format_exception, etype is already ignored and type(value) is used, so I think we could also "just" also ignore tb (unless set) and use value.__traceback__. Leaving the API to function(None, exception)

    I don't see any clean way to migrate to a new API (IMHO a forced exc kwarg is not discoverable enough vs a (None, e, e.traceback)

    A possibility, as etype is already unused, would be replace it with etype_or_exception, and in case it's a full exception use it as the sole parameter from which type and tb get extracted.

    Though that feels weird as well, and the Deprecation Cycles would need to be long.

    Already emitting deprecation warnings (that etype is ignored) would be good.

    @Carreau
    Copy link
    Mannequin

    @Carreau Carreau mannequin commented Feb 27, 2017

    I've attempted a Pull-request trying to implement what Brett is describing. See #327, and opened http://bugs.python.org/issue29660 to document that etype is ignored and decide wether it should emit DeprecationWarning when used.

    @vadmium
    Copy link
    Member

    @vadmium vadmium commented Feb 27, 2017

    Matthias’s proposal adds support for a new keyword-only “exc” argument:

        print_exception(exc=exception_instance)

    I still think it is worth supporting a single positional argument as well:

        print_exception(exception_instance)

    Another point is that it may be better to keep the existing parameter name “value”, rather than (eventually?) replacing it with “exc”. I think both of these things could be accomplished by juggling the “value” and “etype” parameters:

    def print_exception(etype=None, value=None, tb=None, ...):
        if value is None:  # Assume value passed as first positional argument
            value = etype
            etype = None
        if etype is tb is None:  # Only value passed
            etype = type(value)
            tb = value.__traceback__
        # Existing code using (etype, value, tb)

    The disadvantage of any of these changes is that we may want to maintain support for the old signature while Python 2 remains alive.

    @vadmium vadmium added the 3.7 label Feb 27, 2017
    @Carreau
    Copy link
    Mannequin

    @Carreau Carreau mannequin commented Feb 27, 2017

    Another point is that it may be better to keep the existing parameter name “value”, rather than (eventually?) replacing it with “exc”. I think both of these things could be accomplished by juggling the “value” and “etype” parameters:

    I agreed, and that what I would have done outside of CPython, but that felt like this kind of juggling was frowned upon.

    The disadvantage of any of these changes is that we may want to maintain support for the old signature while Python 2 remains alive.

    Your example does not seem to break the old signature. Or I fail to see how.

    If that way would be accepted I'm happy to implement it.

    @brettcannon
    Copy link
    Member Author

    @brettcannon brettcannon commented Feb 27, 2017

    I don't think supporting both approaches is worth it; we should just choose one of them. As for which one, I'm torn. The single argument one is the most pragmatic, but changing types like has always bugged me. But as Martin points out, the raise syntax supports the class or an instance so there's precedent in regards to exceptions themselves. The parameter name is poorly named if we take it in for the first argument which is unfortunate.

    Basically I can't decide. :)

    @terryjreedy
    Copy link
    Member

    @terryjreedy terryjreedy commented Feb 27, 2017

    The print_exception API goes back to when exception values were (or at least, could be) strings. Its doc is incomplete as to the requirements on the args and, at least by 3.5, erroneous. To modify it, we need to understand how it actually works now.

    In 2.7, 'etype' can apparently by anything with an __str__ method.  (Someone could check the 2.7 code.) At least Exception, 'abc', and None result in 'Exception', 'abc' or 'None' leading the last line.  Example:
    >>> try: 1/0
    except Exception as e:
    	tb.print_exception(None, e, None)

    None: integer division or modulo by zero

    By at least 3.5, the etype arg can definitely be anything, because it is ignored. The printed exception type is already grabbed from the exception. Any patch should not change this. Note that this 3.x change already introduced an incompatibility. In 3.5, the above prints 'ZeroDivisionError' instead of 'None'.

    This holdover line in the doc "prints the exception etype and value after the stack trace" is wrong and should be corrected with a 'changed in 3.x' note if done after 3.0.

    In 2.7, value can at least be an exception, a string, or None (not documented). (Ditto for checking the code.)

    >>> try: 1/0
    except Exception as e:
    	tb.print_exception(None, 'zero-divive', tb=None)
    
    None: zero-divive
    >>> try: 1/0
    except Exception as e:
    	tb.print_exception(None, None, None)

    None

    In 3.?, value must be an exception instance (or compatible duck) (not documented).
    ...
    File "C:\Programs\Python36\lib\traceback.py", line 465, in __init__
    if (exc_value and exc_value.__cause__ is not None
    AttributeError: 'str' object has no attribute '__cause__'

    So, more potential incompatibilities with 2.x.

    In 2.7, tb is needed to supply the traceback, or to suppress it. As a separate parameter, it allows a traceback to be modified before printing.* The option of a modified or omitted traceback (the ultimate modification) should be kept.

    *IDLE edits tracebacks before printing to delete artifacts introduced by IDLE internals. The attempt is to print what the console interpreter would. I don't currently know whether it replaces the original on the exception or not. There is also a proposal for the standard interpreter to edit tracebacks after recursion limit exceptions. So traceback editing is useful, and I see no need to force replacement of the semi-private e.__traceback__.

    My suggestions:

    tb: In 3.7, in the API, change 'tb' to 'tb=True'. If tb is left True, grab it from the exception. If tb is explicitly supplied, use it. If tb is set to False or (for back compatibility) None, suppress it.

    value: In 3.5+ document that it must be an exception.
    (Optional) Change 'value' to 'exc' in the API to reflect the 3.x restriction. Document 'value' as a deprecated synonym for keyword usage. Keep the synonym until after 2.7.

    etype: In 3.5+ document that it is an ignored dummy argument and that one can just pass 0, '', or None.
    (Optional) Deprecate the parameter and make it optional. This can be handled* in the code and would be like range having option 'start'. This is messy but would be temporary. Remove after 2.7.

    • 1 arg = exc/value, 3 args are etype, value, tb, 2 args are exc, tb or etype, exc depending on whether the type(first) is BaseException.

    I am inclined to go with both options, but even if the 3 of us agree, I might be inclined to post our intention on pydev.

    @terryjreedy
    Copy link
    Member

    @terryjreedy terryjreedy commented Feb 27, 2017

    Comments probably apply to format_exception and in part, format_exception_only (both also patched in PR237) but I have not checked behavior or code at all.

    @Carreau
    Copy link
    Mannequin

    @Carreau Carreau mannequin commented Mar 3, 2017

    (Optional) Change 'value' to 'exc' in the API to reflect the 3.x restriction Keep the synonym until after 2.7.

    Do you mean after 2.7 EOL, or after 3.7 ?

    etype: In 3.5+ document that it is an ignored dummy argument and that one can just pass 0, '', or None.
    (Optional) Deprecate the parameter and make it optional. This can be handled* in the code and would be like range having option 'start'. This is messy but would be temporary.

    That seem hard to do if we want to allow func(exception), should we attempt to emit a deprecation warning only when etype is used in the Legacy Form ?

    Remove after 2.7.

    Same question as above, unsure what you mean.

    (Optional) Change 'value' to 'exc' in the API to reflect the 3.x restriction. Document 'value' as a deprecated synonym for keyword usage.

    That seem like overly complicated as now the actual exception object can be either in etype, value, or exc.

    Let my try to give example to see if I understood correctly

    2.7 compat

        print_exception(etype, value, tb)
        print_exception(etype=etype, value=value, tb=tb) 

    3.5 compat
    print_exception(None, value, tb)
    print_exception(etype=None, value=value, tb=tb)

    3.7
    print_exception(value) == print_exception(type(value), value, value.__traceback__)
    print_exception(value, tb=True) == print_exception(type(value), value, value.__traceback__)

    print_exception(value, tb=None)     == print_exception(type(value), value, None)
    print_exception(value, tb=False)    == print_exception(type(value), value, None)
    
    
    print_exception(value, tb=faketb)   == print_exception(type(value), value, faketb)
                                  
    # tb can be positional
    
    print_exception(value, tb) == print_exception(value, tb=tb)
    

    Signature would thus be:

        if first param isinstance BaseException: # 3.7+, prefered form
            print_exception(exc, [tb ,] *, [limit, file, chain])
        else:
            print_exception(etype, value, tb, [limit, file, chain])
            # etype being ignored

    Should exc be positional only ?

        print_exception(exc, /, [tb ,] *, [limit, file, chain])

    Should limit, file, chain be allowed to be positional ?

        print_exception(exc, [tb , limit, file, chain])

    @terryjreedy
    Copy link
    Member

    @terryjreedy terryjreedy commented Mar 3, 2017

    Matthias, I will try to respond tomorrow.

    @csabella
    Copy link
    Contributor

    @csabella csabella commented Apr 8, 2019

    The last comment on the original PR for this issue was to wait until an API was decided upon before proceeding with creating a new PR. Bumping this issue to generate new discussion and hopefully reach concession on an API.

    @csabella csabella added 3.8 and removed 3.7 labels Apr 8, 2019
    @brettcannon
    Copy link
    Member Author

    @brettcannon brettcannon commented Apr 10, 2019

    Boy, having a postional-only parameter in that first position would have been handy when we created this API (as Matthias pointed out). :)

    The 'exec' keyword-only parameter is obviously the safest option here.

    Making the first parameter positional-only and then making the other parameters optional would lead to the most fluid API long-term as people I suspect would much rather just pass in an object than always specifying the keyword-only parameter every time going forward. I also doubt anyone is specifying etype by name.

    So my vote is:

    +1 to a positional-only first parameter
    +0 to 'exc' keyword-only parameter

    @csabella
    Copy link
    Contributor

    @csabella csabella commented Oct 20, 2019

    @mbussonn, were you interested in continuing with Brett's (and your) suggestion for the API by making the first argument positional-only and the others optional?

    @csabella csabella added 3.9 and removed 3.8 labels Oct 20, 2019
    @Carreau
    Copy link
    Mannequin

    @Carreau Carreau mannequin commented Nov 14, 2019

    were you interested in continuing with Brett's (and your) suggestion for the API by making the first argument positional-only and the others optional?

    Yes, but I currently do not have the time; if someone else want to take over; or if you need to close this issue or corresponding PR for now please feel free to do so. I'll come back to it at some point.

    @rhettinger
    Copy link
    Contributor

    @rhettinger rhettinger commented Nov 14, 2019

    It would be nice to see this one come to fruition.

    @pablogsal
    Copy link
    Member

    @pablogsal pablogsal commented Nov 5, 2020

    New changeset 91e9379 by Zackery Spytz in branch 'master':
    bpo-26389: Allow passing an exception object in the traceback module (GH-22610)
    91e9379

    @terryjreedy terryjreedy added 3.10 and removed 3.9 labels Feb 12, 2021
    @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
    3.10 stdlib type-feature
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants