classification
Title: trace module: convert to argparse
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: orsenthil Nosy List: SilentGhost, belopolsky, berker.peksag, giampaolo.rodola, orsenthil, python-dev, vajrasky
Priority: normal Keywords: easy, patch

Created on 2014-10-15 12:46 by giampaolo.rodola, last changed 2016-01-13 19:41 by berker.peksag. This issue is now closed.

Files
File name Uploaded Description Edit
better_err_listfuncs_trace.patch vajrasky, 2014-10-19 07:53 review
better_err_listfuncs_trace_v2.patch vajrasky, 2014-10-29 17:10 review
issue22642.diff SilentGhost, 2016-01-12 10:13 review
issue22642_2.diff SilentGhost, 2016-01-12 18:44 review
Messages (13)
msg229441 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2014-10-15 12:46
$ 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
msg229674 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2014-10-19 07:53
Here is the patch.
msg230227 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2014-10-29 17:10
Thanks, Berker Peksag, for the review. Here is the updated patch with the test.
msg257959 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2016-01-11 14:28
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.
msg257960 - (view) Author: SilentGhost (SilentGhost) * (Python triager) Date: 2016-01-11 14:41
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.
msg257963 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2016-01-11 14:57
@SilentGhost, I agree with you. I am making the change to use argparse as part of this issue24649. And this bug could be covered as part of the change to argparse.
msg258097 - (view) Author: SilentGhost (SilentGhost) * (Python triager) Date: 2016-01-12 10:13
Here is the patch that incorporates earlier changes from Vajrasky's patch as well as wording from Evan's patch in issue24649. 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 issue24649.
msg258106 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2016-01-12 15:57
@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!
msg258117 - (view) Author: SilentGhost (SilentGhost) * (Python triager) Date: 2016-01-12 18:44
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.
msg258148 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-01-13 15:47
New changeset 0aa46b9ffba3 by Senthil Kumaran in branch 'default':
Issue22642 - Convert trace module's option handling mechanism from getopt to argparse.
https://hg.python.org/cpython/rev/0aa46b9ffba3
msg258149 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-01-13 15:49
New changeset 69aa17b1f894 by Senthil Kumaran in branch 'default':
Add a NEWS entry for Issue #22642.
https://hg.python.org/cpython/rev/69aa17b1f894
msg258150 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2016-01-13 15:51
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.
msg258162 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2016-01-13 19:41
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 :)
History
Date User Action Args
2016-01-13 19:41:38berker.peksagsetnosy: + berker.peksag
messages: + msg258162
2016-01-13 15:51:23orsenthilsetstatus: open -> closed
resolution: fixed
messages: + msg258150

stage: patch review -> resolved
2016-01-13 15:49:03python-devsetmessages: + msg258149
2016-01-13 15:47:06python-devsetnosy: + python-dev
messages: + msg258148
2016-01-12 18:44:39SilentGhostsetfiles: + issue22642_2.diff

messages: + msg258117
2016-01-12 15:57:22orsenthilsetassignee: orsenthil
messages: + msg258106
versions: - Python 3.5
2016-01-12 10:17:06SilentGhostlinkissue24649 superseder
2016-01-12 10:13:41SilentGhostsetfiles: + issue22642.diff

title: trace module: unclear error message -> trace module: convert to argparse
messages: + msg258097
versions: + Python 3.6, - Python 2.7, Python 3.4
2016-01-11 14:57:05orsenthilsetmessages: + msg257963
2016-01-11 14:41:05SilentGhostsetnosy: + SilentGhost
messages: + msg257960
2016-01-11 14:28:06orsenthilsetnosy: + orsenthil
messages: + msg257959
2014-10-29 17:10:05vajraskysetfiles: + better_err_listfuncs_trace_v2.patch

messages: + msg230227
2014-10-19 16:47:26berker.peksagsettype: behavior
stage: needs patch -> patch review
2014-10-19 07:53:34vajraskysetfiles: + better_err_listfuncs_trace.patch

nosy: + vajrasky
messages: + msg229674

keywords: + patch
2014-10-15 18:43:09ned.deilysetversions: + Python 2.7, Python 3.5
nosy: + belopolsky

components: + Library (Lib)
keywords: + easy
stage: needs patch
2014-10-15 12:46:20giampaolo.rodolacreate