-
-
Notifications
You must be signed in to change notification settings - Fork 33.4k
gh-125115 : Refactor the pdb parsing issue so positional arguments can pass through #140933
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
|
Hey @iritkatriel , I think it's time to do some customized parsing for |
| parser.add_argument('-p', '--pid', type=int, help="attach to the specified PID", default=None) | ||
| parser.print_help() | ||
| sys.exit(2) | ||
| elif args[0] == '-p' or args[0] == '--pid': |
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.
Is -p always the first arg?
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.
So we already skimmed all the -c commands, which are the only "valid options" to all three kinds of invocation. Then we need to determine which path (script, module or pid) we should take. There's no case where -p is not the first argument (after skimming -c) but we should still consider a pid attach.
iritkatriel
left a comment
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.
Probably worth getting a review from @savannahostrowski as the argparse maintainer.
savannahostrowski
left a comment
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 think custom parsing logic is necessary here, given that parse_known_args is too greedy and REMAINDER is plagued with issues (see #58382 as an example). There's no combination of argparse features that will make this work, AFAIK.
On a more meta note, it does make me wonder whether it'd be worth exposing a proper passthrough parameter in argparse itself to simplify things, especially since this scenario is actually not that uncommon.
|
Hi @savannahostrowski and @iritkatriel , any more comments on this PR? |
|
Thanks @gaogaotiantian for the PR 🌮🎉.. I'm working now to backport this PR to: 3.14. |
|
Sorry, @gaogaotiantian, I could not cleanly backport this to |
|
GH-141635 is a backport of this pull request to the 3.14 branch. |
We are at a point that we need to do customize parsing for pdb in order to make the positional argument passing intuitive.
For example
The order of the arguments now matter (especially when we use a very common option
-p).Now pdb try to
-ccommands (for backward compatible reason it still get commands after the script/module).-c).