-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
gh-109164: Replace getopt
with argparse
in pdb
#109165
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some suggestions:
- Disable allow_abbrev
- Use a mutually exclusive argument group for
-m
andpyfile
- 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:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
A
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO nargs='?'
is wrong. You cannot pass no pyfile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just counter intuitive to me.
This bit of argparse's design is odd, I agree.
A
target = _ModuleTarget(file) | ||
else: | ||
file = opts.pyfile | ||
target = _ScriptTarget(file) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be a larger refactor, the current state reflects what's currently present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, that's why I'm asking for :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just disturbed by calling "module" a "file".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
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
argparse
makes the code cleaner and easier to read (especially to people who are already familiar withargparse
).getopt
withargparse
in pdb CLI #109164