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

trace module: convert to argparse #66832

Closed
giampaolo opened this issue Oct 15, 2014 · 13 comments
Closed

trace module: convert to argparse #66832

giampaolo opened this issue Oct 15, 2014 · 13 comments
Assignees
Labels
easy stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@giampaolo
Copy link
Contributor

BPO 22642
Nosy @abalkin, @orsenthil, @giampaolo, @berkerpeksag, @vajrasky
Files
  • better_err_listfuncs_trace.patch
  • better_err_listfuncs_trace_v2.patch
  • issue22642.diff
  • issue22642_2.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/orsenthil'
    closed_at = <Date 2016-01-13.15:51:23.721>
    created_at = <Date 2014-10-15.12:46:20.804>
    labels = ['easy', 'type-bug', 'library']
    title = 'trace module: convert to argparse'
    updated_at = <Date 2016-01-13.19:41:38.721>
    user = 'https://github.com/giampaolo'

    bugs.python.org fields:

    activity = <Date 2016-01-13.19:41:38.721>
    actor = 'berker.peksag'
    assignee = 'orsenthil'
    closed = True
    closed_date = <Date 2016-01-13.15:51:23.721>
    closer = 'orsenthil'
    components = ['Library (Lib)']
    creation = <Date 2014-10-15.12:46:20.804>
    creator = 'giampaolo.rodola'
    dependencies = []
    files = ['36967', '37068', '41592', '41594']
    hgrepos = []
    issue_num = 22642
    keywords = ['patch', 'easy']
    message_count = 13.0
    messages = ['229441', '229674', '230227', '257959', '257960', '257963', '258097', '258106', '258117', '258148', '258149', '258150', '258162']
    nosy_count = 7.0
    nosy_names = ['belopolsky', 'orsenthil', 'giampaolo.rodola', 'SilentGhost', 'python-dev', 'berker.peksag', 'vajrasky']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue22642'
    versions = ['Python 3.6']

    @giampaolo
    Copy link
    Contributor Author

    $ python3.4 -m trace -l 
    Traceback (most recent call last):
      File "/usr/local/lib/python3.4/runpy.py", line 170, in _run_module_as_main
        "__main__", mod_spec)
      File "/usr/local/lib/python3.4/runpy.py", line 85, in _run_code
        exec(code, run_globals)
      File "/usr/local/lib/python3.4/trace.py", line 858, in <module>
        main()
      File "/usr/local/lib/python3.4/trace.py", line 787, in main
        progname = prog_argv[0]
    IndexError: list index out of range

    I would expect something more clear to be printed, like the program usage helper you get in this case:

    $ python3.4 -m trace 
    /usr/local/lib/python3.4/trace.py: must specify one of --trace, --count, --report, --listfuncs, or --trackcalls

    @ned-deily ned-deily added easy stdlib Python modules in the Lib dir labels Oct 15, 2014
    @vajrasky
    Copy link
    Mannequin

    vajrasky mannequin commented Oct 19, 2014

    Here is the patch.

    @berkerpeksag berkerpeksag added the type-bug An unexpected behavior, bug, or error label Oct 19, 2014
    @vajrasky
    Copy link
    Mannequin

    vajrasky mannequin commented Oct 29, 2014

    Thanks, Berker Peksag, for the review. Here is the updated patch with the test.

    @orsenthil
    Copy link
    Member

    I reviewed the patch. It looks like the problem is not just with list functions but with other options too. Like.

    $ ./python.exe -m trace -t
    $ ./python.exe -m trace -c
    $ ./python.exe -m trace -T

    Will throw the same error. So, any changes should address all these cases instead of a single listfunctions (-l) case only.

    @SilentGhost
    Copy link
    Mannequin

    SilentGhost mannequin commented Jan 11, 2016

    I could submit a patch, but I'd switch over from getopt to argparse. I think this exactly the case when such a change is warranted.

    @orsenthil
    Copy link
    Member

    @SilentGhost, I agree with you. I am making the change to use argparse as part of this bpo-24649. And this bug could be covered as part of the change to argparse.

    @SilentGhost
    Copy link
    Mannequin

    SilentGhost mannequin commented Jan 12, 2016

    Here is the patch that incorporates earlier changes from Vajrasky's patch as well as wording from Evan's patch in bpo-24649. I've extended the test of failures, but I'm not sure if much can be done about the test of successful executions. I'm going to mark this issue as a superseder of the bpo-24649.

    @SilentGhost SilentGhost mannequin changed the title trace module: unclear error message trace module: convert to argparse Jan 12, 2016
    @orsenthil
    Copy link
    Member

    @SilentGhost,

    Thanks a lot for the quick turn around of a patch. I have left some review comments in reitveld. Please address them.

    Since this is underlying implementation change in option processing/parsing, it should only in feature release (3.6).

    Thank you!

    @orsenthil orsenthil self-assigned this Jan 12, 2016
    @SilentGhost
    Copy link
    Mannequin

    SilentGhost mannequin commented Jan 12, 2016

    Here is the updated version of the patch that addresses Senthil's comments. Besides re-flowing code, I've also added enforcement of --summary switch and re-factored and expanded the test_failures. It probably would be good if someone could test this on a real-life code.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 13, 2016

    New changeset 0aa46b9ffba3 by Senthil Kumaran in branch 'default':
    bpo-22642 - Convert trace module's option handling mechanism from getopt to argparse.
    https://hg.python.org/cpython/rev/0aa46b9ffba3

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 13, 2016

    New changeset 69aa17b1f894 by Senthil Kumaran in branch 'default':
    Add a NEWS entry for Issue bpo-22642.
    https://hg.python.org/cpython/rev/69aa17b1f894

    @orsenthil
    Copy link
    Member

    Thanks for the contribution. The option handling of trace module is modernized now.

    As berker suggested in one of the review comments, this module could see an increase in test coverage and we could deal with this as separate ticket.

    @berkerpeksag
    Copy link
    Member

    Thanks, Senthil. My comment was for Vajrasky's patch, not for the whole argparse switch. We need to add tests for the old getopt code first to avoid regressions. After 0aa46b9ffba3, if we add tests we won't be able to test the old interface.

    My other comments:

    • SilentGhost needs to fill the contributor's agreement form. We generally accept trivial patches without asking CA, but I think we can't accept 0aa46b9ffba3 without one since the patch is huge.
    • I don't think we add reST formatted docstring to stdlib. The javadoc style one in Trace.__init__() is 13 years old so we can ignore it.
    • I also have some minor comments, but I don't think they are important at this point :)

    @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
    easy stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants