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

Expose capture_locals parameter in traceback convenience functions #77990

Closed
ulope mannequin opened this issue Jun 8, 2018 · 14 comments
Closed

Expose capture_locals parameter in traceback convenience functions #77990

ulope mannequin opened this issue Jun 8, 2018 · 14 comments
Labels
3.11 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@ulope
Copy link
Mannequin

ulope mannequin commented Jun 8, 2018

BPO 33809
Nosy @bitdancer, @farhaanbukhsh, @iritkatriel
PRs
  • bpo-33809: add the TracebackException.print() method #24231
  • 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 2021-05-22.16:41:50.866>
    created_at = <Date 2018-06-08.15:52:20.399>
    labels = ['type-feature', 'library', '3.11']
    title = 'Expose `capture_locals` parameter in `traceback` convenience functions'
    updated_at = <Date 2021-09-16.18:04:41.193>
    user = 'https://bugs.python.org/ulope'

    bugs.python.org fields:

    activity = <Date 2021-09-16.18:04:41.193>
    actor = 'iritkatriel'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-05-22.16:41:50.866>
    closer = 'iritkatriel'
    components = ['Library (Lib)']
    creation = <Date 2018-06-08.15:52:20.399>
    creator = 'ulope'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 33809
    keywords = ['patch']
    message_count = 14.0
    messages = ['319079', '319809', '320176', '320191', '385028', '385029', '385030', '385055', '385056', '385089', '385106', '385108', '401975', '401976']
    nosy_count = 4.0
    nosy_names = ['r.david.murray', 'ulope', 'fhackdroid', 'iritkatriel']
    pr_nums = ['24231']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue33809'
    versions = ['Python 3.11']

    @ulope
    Copy link
    Mannequin Author

    ulope mannequin commented Jun 8, 2018

    Since 3.5 the internal machinery of the traceback module has gained the very useful ability to capture locals.

    It would be useful to also expose that ability through the various convenience functions.

    @ulope ulope mannequin added 3.7 (EOL) end of life 3.8 only security fixes stdlib Python modules in the Lib dir labels Jun 8, 2018
    @farhaanbukhsh
    Copy link
    Mannequin

    farhaanbukhsh mannequin commented Jun 17, 2018

    Hey, I would like to work on this, where do I start or how can I help?

    @bitdancer
    Copy link
    Member

    I think we should get one or more of the core devs who were involved in the changes that added that parameter to sign off on whether this is a good idea or not (I have no opinion at the moment). You should be able to find them via git blame and looking at the issues related to the changesets you find. Unless someone who remembers comes along and just adds them to nosy :)

    @bitdancer bitdancer added type-feature A feature request or enhancement and removed 3.7 (EOL) end of life labels Jun 21, 2018
    @farhaanbukhsh
    Copy link
    Mannequin

    farhaanbukhsh mannequin commented Jun 21, 2018

    Hahah! Let me try to put this on the IRC channel may be someone can help us there.

    @iritkatriel
    Copy link
    Member

    I don't understand this request. If you ask TracebackExceptions to capture_locals then the FrameSummary object gets a dict of the reprs of the locals, which can be introspected and participate in __eq__ checks but nothing else happens with them. What would the convenience functions do with this parameter?

    Ulrich, can you explain the use case or how you see this working?

    @iritkatriel
    Copy link
    Member

    Sorry, I spoke too soon - see now that the locals are use in the StackSummary.format().

    @iritkatriel
    Copy link
    Member

    That said, you can always use

    TracebackException.from_exception(exc, capture_locals=True).format()

    which is not much longer than

    print_exception(exc, capture_locals=True)

    @ulope
    Copy link
    Mannequin Author

    ulope mannequin commented Jan 13, 2021

    Functionally equivalent code would be:

    print("".join(TracebackException.from_exception(ex, capture_locals=True).format()))

    vs. (hypothetically)

    print_exc(capture_locals=True)

    Which is quite a significant difference IMO.

    @iritkatriel
    Copy link
    Member

    I’m not sure I agree that it’s a big win.
    You can always add such a utility function in your code.

    @iritkatriel
    Copy link
    Member

    On the other hand, I think it makes sense to add a print() method to TracebackException so that you can do

    TracebackException.from_exception(ex, capture_locals=True).print(file)

    or whatever other combination of current or future params.

    @ulope
    Copy link
    Mannequin Author

    ulope mannequin commented Jan 15, 2021

    That would make it slightly better, but I still think it's a pretty arcane incantation (esp. for newer people).

    What makes you hesitant to adding the parameter to the convenience functions?

    @iritkatriel
    Copy link
    Member

    Generally speaking, I don't think it's a good idea to create redundant APIs. If we copy all the params from TracebackException (and it will be all params, why only this one?) that means more code, more tests, more documentation and a higher cognitive load for someone reading the documentation to learn the API. And the users don't really get anything tangible in return - they can do exactly the same things, maybe with a slightly smaller number of characters. A good standard library API has a small number of buttons that can be used in combination to get a lot of functionality.

    In this particular case, I think it's nice that print_exception() gives novices a simple way to get some basic functionality, and more sophisticated users can go to TracebackException for more options. I agree with you that at the moment it's a bit clumsy, and a print() method would make it more usable. Note that a programmer who understands the different parts of a traceback and wants to tweak its representation is not a novice and should be able to use that with ease.

    @iritkatriel iritkatriel added 3.10 only security fixes and removed 3.8 only security fixes labels Jan 15, 2021
    @iritkatriel iritkatriel added 3.11 only security fixes and removed 3.10 only security fixes labels May 12, 2021
    @ulope
    Copy link
    Mannequin Author

    ulope mannequin commented Sep 16, 2021

    If we copy all the params from TracebackException (and it will be all params, why only this one?)

    Why expose the internal machinery at all?
    If all additional parameters (and unless I'm miscounting we're talking about 2) were exposed on the convenience functions why add to the developers mental load with the underlying implementation?

    more documentation and a higher cognitive load for someone reading the documentation to learn the API.

    I would agree if the API in question was ergonomic and the documentation easy to read in the first place. I think neither is true for the traceback module and its documentation.

    Another example from today where I helped a coworker who was unable to figure out on his own how to print the current stack with locals:

      from traceback import StackSummary, walk_stack
      
      print("".join(StackSummary.extract(walk_stack(None), capture_locals=True).format()))

    There are multiple things that make this API non-intuitive:

    • Why does walk_stack() need an explicit None argument?
    • Why is StackSummary created via the extract() classmethod instead of having a regular __init__?
    • Why can't extract() be smart if no iterator is passed and figure out on it's own if we're in an exception context or not and call walk_stack / walk_tb accordingly?
    • Why is the result a list if each item can contain multiple internal newlines?

    @iritkatriel
    Copy link
    Member

    Feel free to create a new issue and propose a patch.

    @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.11 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants