Skip to content
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.

Sign up for GitHub

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

Jump to bottom

gh-109164: Replace getopt with argparse in pdb #109165

Merged
merged 7 commits into from
Sep 22, 2023
Merged

gh-109164: Replace getopt with argparse in pdb #109165

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a9d0109
Replace getopt with argparse in pdb
gaogaotiantian Sep 8, 2023
6c5bb11
📜🤖 Added by blurb_it.
blurb-it[bot] Sep 8, 2023
d51219c
Apply suggestions from code review
gaogaotiantian Sep 10, 2023
e417507
Rename & fix a bug
gaogaotiantian Sep 10, 2023
cdf3570
Add comments about why we need to check argv
gaogaotiantian Sep 16, 2023
2defa6b
Update Lib/pdb.py
gaogaotiantian Sep 18, 2023
f14bc09
Update changelog entry
AA-Turner Sep 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
38 changes: 22 additions & 16 deletions Lib/pdb.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -2077,34 +2075,42 @@ def help():


def main():
import getopt
import argparse

opts, args = getopt.getopt(sys.argv[1:], 'mhc:', ['help', 'command='])
parser = argparse.ArgumentParser(prog="pdb",
description=_usage,
formatter_class=argparse.RawDescriptionHelpFormatter,
allow_abbrev=False)

if not args:
print(_usage)
sys.exit(2)
parser.add_argument('-c', '--command', action='append', default=[], metavar='command')
Copy link
Member

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.

group = parser.add_mutually_exclusive_group(required=True)
group.add_argument('-m', metavar='module')
Copy link
Member

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.

Suggested change
group.add_argument('-m', metavar='module')
group.add_argument('-m', metavar='module', dest='module')

group.add_argument('pyfile', nargs='?')
Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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

Copy link
Member

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

Copy link
Member

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

parser.add_argument('args', nargs="*")

if any(opt in ['-h', '--help'] for opt, optarg in opts):
print(_usage)
sys.exit()
if len(sys.argv) == 1:
AA-Turner marked this conversation as resolved.
Show resolved Hide resolved
parser.print_help()
sys.exit(2)

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)

Copy link
Member

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

Copy link
Member

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.

Copy link
Member

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

Copy link
Member Author

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.

Copy link
Member

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".

Copy link
Member

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.

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)
Expand Down
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Library/2023-09-08-22-26-26.gh-issue-109164.-9BFWR.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Replace getopt with argparse for parsing arguments in :mod:`pdb`
AA-Turner marked this conversation as resolved.
Show resolved Hide resolved