trace module: convert to argparse

[giampaolo]

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]

$ 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

[vajrasky]

Here is the patch.

[vajrasky]

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

[orsenthil]

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]

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]

@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]

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.

[orsenthil]

@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!

[SilentGhost]

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]

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]

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]

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]

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