gh-109164: Replace getopt with argparse in pdb

[gaogaotiantian]

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

• Issue: Replace the old getopt with argparse in pdb CLI #109164

[AA-Turner]

Some suggestions:

• Disable allow_abbrev
• Use a mutually exclusive argument group for -m and pyfile
• Add metavars to mimic the old help text

A

[gaogaotiantian]

Some suggestions:

• Disable allow_abbrev
• Use a mutually exclusive argument group for -m and pyfile
• Add metavars to mimic the old help text

A

Thank you, it does make more sense in this way.

I applied your changes with some modifications:

• I do prefer group than grp - full name when the word is not that long, in line with parser, file and targets (though opts is abbr, I don't hate it that much)
• _ScriptTarget(opts.pyfile[0]) would be wrong now as it's a character now. However, we should use file anyway so I just used that.

[AA-Turner]

Thanks!

A

[vstinner]

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

[vstinner]

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.

[vstinner]

IMO nargs='?' is wrong. You cannot pass no pyfile.

[AA-Turner]

This is a slight work-around for limitations in argparse -- we want the mutually exclusive group behaviour, so nargs='?' is needed.

[vstinner]

Which limitation? Is there is a reason to use the wrong nargs on purpose, please add a comment to explain why.

[gaogaotiantian]

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.

[vstinner]

That's not how exclusive group works. Either you pass a module or a pyfile. pyfile is not optional.

[gaogaotiantian]

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

[vstinner]

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

[AA-Turner]

It's just counter intuitive to me.

This bit of argparse's design is odd, I agree.

A

[vstinner]

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

[AA-Turner]

This would be a larger refactor, the current state reflects what's currently present.

[vstinner]

Right, that's why I'm asking for :-)

[gaogaotiantian]

The logic is pretty similar right? Just a slightly different implementation. I don't think this is too much if this is preferred.

[vstinner]

I'm just disturbed by calling "module" a "file".

[vstinner]

Also, feel free to ignore my comment about this code.

[vstinner]

LGTM.

[gaogaotiantian]

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.