gh-109164: Replace getopt with argparse in pdb#109165
Conversation
AA-Turner
left a comment
There was a problem hiding this comment.
Some suggestions:
- Disable allow_abbrev
- Use a mutually exclusive argument group for
-mandpyfile - Add metavars to mimic the old help text
A
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
Thank you, it does make more sense in this way. I applied your changes with some modifications:
|
Co-authored-by: Victor Stinner <vstinner@python.org>
|
|
||
| parser.add_argument('-c', '--command', action='append', default=[], metavar='command') | ||
| group = parser.add_mutually_exclusive_group(required=True) | ||
| group.add_argument('-m', metavar='module') |
There was a problem hiding this comment.
I suggest using names longer than 1 letter, you should update the code below as well.
| group.add_argument('-m', metavar='module') | |
| group.add_argument('-m', metavar='module', dest='module') |
| formatter_class=argparse.RawDescriptionHelpFormatter, | ||
| allow_abbrev=False) | ||
|
|
||
| parser.add_argument('-c', '--command', action='append', default=[], metavar='command') |
There was a problem hiding this comment.
It's surprising that -c looks like python -c option, but different. Would you mind to add a help message to explain that there are pdb commands? And that they have the same format than .pdbrc configuration files?
I suggest to add dest='commands', since it's non-obvious from command name that's a list.
| parser.add_argument('-c', '--command', action='append', default=[], metavar='command') | ||
| group = parser.add_mutually_exclusive_group(required=True) | ||
| group.add_argument('-m', metavar='module') | ||
| group.add_argument('pyfile', nargs='?') |
There was a problem hiding this comment.
IMO nargs='?' is wrong. You cannot pass no pyfile.
There was a problem hiding this comment.
This is a slight work-around for limitations in argparse -- we want the mutually exclusive group behaviour, so nargs='?' is needed.
There was a problem hiding this comment.
Which limitation? Is there is a reason to use the wrong nargs on purpose, please add a comment to explain why.
There was a problem hiding this comment.
Which limitation? Is there is a reason to use the wrong nargs on purpose, please add a comment to explain why.
I don't think this is wrong. Consider it as - you can either pass a module with -m OR a pyfile. So pyfile is optional, you can pass no pyfile.
There was a problem hiding this comment.
That's not how exclusive group works. Either you pass a module or a pyfile. pyfile is not optional.
There was a problem hiding this comment.
That's not how exclusive group works. Either you pass a module or a pyfile. pyfile is not optional.
I think either pass a module or a pyfile -> pyfile is optional, it's literally in a either/or sentence.
Mutually exclusive group requires all the options in the group to be "optional". What's your proposal for this? You can't use nargs=1 because that would make pyfile a required argument - and it's not. It's absolutely fine if you don't pass a pyfile when you pass a module (actually you can't pass a pyfile when you pass a module).
There was a problem hiding this comment.
Sorry, I misunderstood how exclusive groups work. It's kind of surprising. Apparently, you must use nargs='?'. It's just counter intuitive to me.
import argparse
parser = argparse.ArgumentParser(prog='PROG')
group = parser.add_argument_group()
exclusive_group = group.add_mutually_exclusive_group(required=True)
exclusive_group.add_argument('-m', metavar='MODULE')
exclusive_group.add_argument('pyscript', nargs='?')
print(parser.parse_args(['-m', 'mymod']))
print(parser.parse_args(['myscript']))
print(parser.parse_args([]))Output:
$ python3 x.py
Namespace(m='mymod', pyscript=None)
Namespace(m=None, pyscript='myscript')
usage: PROG [-h] (-m MODULE | pyscript)
PROG: error: one of the arguments -m pyscript is required
There was a problem hiding this comment.
It's just counter intuitive to me.
This bit of argparse's design is odd, I agree.
A
| else: | ||
| file = opts.pyfile | ||
| target = _ScriptTarget(file) | ||
|
|
There was a problem hiding this comment.
I would prefer:
if opts.module:
args = [opts.module]
...
else:
args = [opts.pyfile]
...
args.extend(opts.ags)
...
sys.argv = args # Hide "pdb.py" and pdb options from argument list
There was a problem hiding this comment.
This would be a larger refactor, the current state reflects what's currently present.
There was a problem hiding this comment.
Right, that's why I'm asking for :-)
There was a problem hiding this comment.
The logic is pretty similar right? Just a slightly different implementation. I don't think this is too much if this is preferred.
There was a problem hiding this comment.
I'm just disturbed by calling "module" a "file".
There was a problem hiding this comment.
Also, feel free to ignore my comment about this code.
Misc/NEWS.d/next/Library/2023-09-08-22-26-26.gh-issue-109164.-9BFWR.rst
Outdated
Show resolved
Hide resolved
Co-authored-by: Victor Stinner <vstinner@python.org>
|
Ah, sorry I did not have the chance to address some of the comments which I believe are valid. (comment -> comments for example). We can leave it here for now and there will be the next refactor. |
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com> Co-authored-by: Victor Stinner <vstinner@python.org>
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com> Co-authored-by: Victor Stinner <vstinner@python.org>
The behavior is almost identical - except for the minor difference in help message. Switching to
argparsemakes the code cleaner and easier to read (especially to people who are already familiar withargparse).getoptwithargparsein pdb CLI #109164