-
-
Notifications
You must be signed in to change notification settings - Fork 29.2k
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
gh-109164: Replace getopt
with argparse
in pdb
#109165
Changes from all commits
a9d0109
6c5bb11
d51219c
e417507
cdf3570
2defa6b
f14bc09
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -2061,8 +2061,6 @@ def help(): | |||||
pydoc.pager(__doc__) | ||||||
|
||||||
_usage = """\ | ||||||
usage: pdb.py [-c command] ... [-m module | pyfile] [arg] ... | ||||||
|
||||||
Debug the Python program given by pyfile. Alternatively, | ||||||
an executable module or package to debug can be specified using | ||||||
the -m switch. | ||||||
|
@@ -2077,34 +2075,44 @@ def help(): | |||||
|
||||||
|
||||||
def main(): | ||||||
import getopt | ||||||
|
||||||
opts, args = getopt.getopt(sys.argv[1:], 'mhc:', ['help', 'command=']) | ||||||
|
||||||
if not args: | ||||||
print(_usage) | ||||||
import argparse | ||||||
|
||||||
parser = argparse.ArgumentParser(prog="pdb", | ||||||
description=_usage, | ||||||
formatter_class=argparse.RawDescriptionHelpFormatter, | ||||||
allow_abbrev=False) | ||||||
|
||||||
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 commentThe 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.
Suggested change
|
||||||
group.add_argument('pyfile', nargs='?') | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more.
I don't think this is wrong. Consider it as - you can either pass a module with There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more.
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This bit of argparse's design is odd, I agree. A |
||||||
parser.add_argument('args', nargs="*") | ||||||
|
||||||
if len(sys.argv) == 1: | ||||||
AA-Turner marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
# If no arguments were given (python -m pdb), print the whole help message. | ||||||
# Without this check, argparse would only complain about missing required arguments. | ||||||
parser.print_help() | ||||||
sys.exit(2) | ||||||
|
||||||
if any(opt in ['-h', '--help'] for opt, optarg in opts): | ||||||
print(_usage) | ||||||
sys.exit() | ||||||
|
||||||
commands = [optarg for opt, optarg in opts if opt in ['-c', '--command']] | ||||||
opts = parser.parse_args() | ||||||
|
||||||
module_indicated = any(opt in ['-m'] for opt, optarg in opts) | ||||||
cls = _ModuleTarget if module_indicated else _ScriptTarget | ||||||
target = cls(args[0]) | ||||||
if opts.m: | ||||||
file = opts.m | ||||||
target = _ModuleTarget(file) | ||||||
else: | ||||||
file = opts.pyfile | ||||||
target = _ScriptTarget(file) | ||||||
|
||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer:
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Also, feel free to ignore my comment about this code. |
||||||
target.check() | ||||||
|
||||||
sys.argv[:] = args # Hide "pdb.py" and pdb options from argument list | ||||||
sys.argv[:] = [file] + opts.args # Hide "pdb.py" and pdb options from argument list | ||||||
|
||||||
# Note on saving/restoring sys.argv: it's a good idea when sys.argv was | ||||||
# modified by the script being debugged. It's a bad idea when it was | ||||||
# changed by the user from the command line. There is a "restart" command | ||||||
# which allows explicit specification of command line arguments. | ||||||
pdb = Pdb() | ||||||
pdb.rcLines.extend(commands) | ||||||
pdb.rcLines.extend(opts.command) | ||||||
while True: | ||||||
try: | ||||||
pdb._run(target) | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
:mod:`pdb`: Replace :mod:`getopt` with :mod:`argparse` for parsing command line arguments. |
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 fromcommand
name that's a list.