GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-109164: Replace `getopt` with `argparse` in pdb by gaogaotiantian · Pull Request #109165 · python/cpython

Conversation

@gaogaotiantian

@gaogaotiantian gaogaotiantian commented Sep 8, 2023 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@gaogaotiantian
Replace getopt with argparse in pdb
a9d0109
@gaogaotiantian
gaogaotiantian requested a review from iritkatriel September 8, 2023 22:25
@bedevere-bot bedevere-bot added the awaiting review label Sep 8, 2023
@bedevere-bot bedevere-bot mentioned this pull request Sep 8, 2023
Replace the old getopt with argparse in pdb CLI #109164
Closed
@blurb-it
📜🤖 Added by blurb_it.
6c5bb11
SylvainDe
SylvainDe reviewed Sep 9, 2023
View reviewed changes
Comment thread Lib/pdb.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
AA-Turner
AA-Turner reviewed Sep 10, 2023
View reviewed changes

@AA-Turner AA-Turner left a comment

Copy link
Copy Markdown
Member

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 and pyfile
  • Add metavars to mimic the old help text

A

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/pdb.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/pdb.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/pdb.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/pdb.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
gaogaotiantian and others added 2 commits September 9, 2023 22:50
@gaogaotiantian @AA-Turner
Apply suggestions from code review
d51219c 
Co-authored-by: Adam Turner <[email protected]>
@gaogaotiantian
Rename & fix a bug
e417507
@gaogaotiantian

gaogaotiantian commented Sep 10, 2023

Copy link
Copy Markdown
Member Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gaogaotiantian
gaogaotiantian requested a review from AA-Turner September 12, 2023 20:47
@brandtbucher
brandtbucher self-requested a review September 12, 2023 21:23
AA-Turner
AA-Turner approved these changes Sep 13, 2023
View reviewed changes

@AA-Turner AA-Turner left a comment

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-app bedevere-app Bot added awaiting core review and removed awaiting review labels Sep 13, 2023
@gaogaotiantian
Add comments about why we need to check argv
cdf3570
vstinner
vstinner reviewed Sep 18, 2023
View reviewed changes
Comment thread Lib/pdb.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@gaogaotiantian @vstinner
Update Lib/pdb.py
2defa6b 
Co-authored-by: Victor Stinner <[email protected]>
vstinner
vstinner reviewed Sep 19, 2023
View reviewed changes
Comment thread Lib/pdb.py

parser.add_argument('-c', '--command', action='append', default=[], metavar='command')
group = parser.add_mutually_exclusive_group(required=True)
group.add_argument('-m', metavar='module')

@vstinner vstinner Sep 19, 2023

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/pdb.py
formatter_class=argparse.RawDescriptionHelpFormatter,
allow_abbrev=False)

parser.add_argument('-c', '--command', action='append', default=[], metavar='command')

@vstinner vstinner Sep 19, 2023

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/pdb.py
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='?')

@vstinner vstinner Sep 19, 2023

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@AA-Turner AA-Turner Sep 19, 2023

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner Sep 19, 2023

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@gaogaotiantian gaogaotiantian Sep 19, 2023

Copy link
Copy Markdown
Member Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner Sep 19, 2023

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@gaogaotiantian gaogaotiantian Sep 19, 2023

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner Sep 19, 2023

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
AA-Turner reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@AA-Turner AA-Turner Sep 20, 2023

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/pdb.py
else:
file = opts.pyfile
target = _ScriptTarget(file)

@vstinner vstinner Sep 19, 2023

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@AA-Turner AA-Turner Sep 19, 2023

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner Sep 19, 2023

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@gaogaotiantian gaogaotiantian Sep 19, 2023

Copy link
Copy Markdown
Member Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
AA-Turner reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@vstinner vstinner Sep 19, 2023

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner Sep 20, 2023

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Misc/NEWS.d/next/Library/2023-09-08-22-26-26.gh-issue-109164.-9BFWR.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
@AA-Turner @vstinner
Update changelog entry
f14bc09 
Co-authored-by: Victor Stinner <[email protected]>
vstinner
vstinner approved these changes Sep 22, 2023
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
AA-Turner reacted with hooray emoji
All reactions
  • 🎉 1 reaction
@vstinner
vstinner enabled auto-merge (squash) September 22, 2023 16:52
@vstinner
vstinner merged commit 73ccfa2 into python:main Sep 22, 2023
@bedevere-app bedevere-app Bot removed the awaiting core review label Sep 22, 2023
@gaogaotiantian
gaogaotiantian deleted the pdb-argparse branch September 22, 2023 17:43
@gaogaotiantian

gaogaotiantian commented Sep 22, 2023

Copy link
Copy Markdown
Member Author

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.
vstinner reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
csm10495 pushed a commit to csm10495/cpython that referenced this pull request Sep 28, 2023
@gaogaotiantian @AA-Turner
@vstinner @csm10495
pythongh-109164: Replace getopt with argparse in pdb (python#109165)
7ae8f6b 
Co-authored-by: Adam Turner <[email protected]>
Co-authored-by: Victor Stinner <[email protected]>
@chgnrdv chgnrdv mentioned this pull request Oct 16, 2023
[3.12] gh-108791: Fix pdb CLI invalid argument handling (GH-108816) #110915
Closed
@vivster7 vivster7 mentioned this pull request Dec 28, 2023
pdb CLI argument parsing errors with arguments to module targets #113548
Closed
@gaogaotiantian gaogaotiantian mentioned this pull request Dec 28, 2023
gh-113548: Allow CLI arguments to pdb -m #113557
Merged
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
@gaogaotiantian @AA-Turner
@vstinner @Glyphack
pythongh-109164: Replace getopt with argparse in pdb (python#109165)
ad4e824 
Co-authored-by: Adam Turner <[email protected]>
Co-authored-by: Victor Stinner <[email protected]>
@SnoopJ SnoopJ mentioned this pull request Jun 13, 2025
pdb consumes arguments greedily #135490
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AA-Turner AA-Turner AA-Turner approved these changes
@vstinner vstinner vstinner approved these changes
@iritkatriel iritkatriel Awaiting requested review from iritkatriel
@brandtbucher brandtbucher Awaiting requested review from brandtbucher
+1 more reviewer
@SylvainDe SylvainDe SylvainDe left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@gaogaotiantian @vstinner @SylvainDe @AA-Turner @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.