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.

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-125355: Rewrite parse_intermixed_args() in argparse #125356

Merged

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Oct 12, 2024

  • The parser no longer changes temporarily during parsing.
  • Default values are not processed twice.
  • Required mutually exclusive groups containing positional arguments are now supported.
  • The missing arguments report now includes the names of all required optional and positional arguments.
  • Unknown options can be intermixed with positional arguments in parse_known_intermixed_args().

* The parser is no longer temporary modified and can be used
  concurrently.
* Default values are no long handled twice.
* Report about missed arguments now contains names of all required optional
  and positional arguments.
* Unknown options can be intermixed with positionals in
  parse_known_intermixed_args().
@@ -6350,7 +6356,7 @@ def test_missing_argument_name_in_message(self):
with self.assertRaises(ArgumentParserError) as cm:
parser.parse_intermixed_args([])
msg = str(cm.exception)
self.assertNotRegex(msg, 'req_pos')
self.assertRegex(msg, 'req_pos')
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a bug.

Comment on lines -6284 to -6420
self.assertEqual(NS(bar='y', cmd='cmd', foo='x', rest=[1]), args)
self.assertEqual(['--error', '2', '3'], extras)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that was a bug. The result was different depending on the position of unknown option.

Comment on lines -6330 to -6469
parser = ErrorRaisingArgumentParser(prog='PROG')
parser.add_argument('--foo', nargs="*")
parser.add_argument('foo')
with self.assertWarns(UserWarning):
parser.parse_intermixed_args(['hello', '--foo'])
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test was added years after implementing this feature (see 9efaff5/#103570) just to increase coverage. I am not sure that it tests the right case, and I do not understand why the warning was emitted at first place.

Comment on lines -2487 to -2510
if (hasattr(namespace, action.dest)
and getattr(namespace, action.dest)==[]):
from warnings import warn
warn('Do not expect %s in %s' % (action.dest, namespace))
delattr(namespace, action.dest)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not understand this code. There were no tests, and the purpose is not clear to me. @hpaulj, @bitdancer, could you shed some light on this?

@savannahostrowski savannahostrowski linked an issue Oct 18, 2024 that may be closed by this pull request
@serhiy-storchaka serhiy-storchaka marked this pull request as ready for review October 21, 2024 20:03
@serhiy-storchaka serhiy-storchaka enabled auto-merge (squash) October 22, 2024 10:31
@serhiy-storchaka serhiy-storchaka added needs backport to 3.12 bug and security fixes needs backport to 3.13 bugs and security fixes labels Oct 22, 2024
@serhiy-storchaka serhiy-storchaka merged commit 759a54d into python:main Oct 22, 2024
37 checks passed
@miss-islington-app
Copy link

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 22, 2024
…H-125356)

* The parser no longer changes temporarily during parsing.
* Default values are not processed twice.
* Required mutually exclusive groups containing positional arguments are
  now supported.
* The missing arguments report now includes the names of all required
  optional and positional arguments.
* Unknown options can be intermixed with positional arguments in
  parse_known_intermixed_args().
(cherry picked from commit 759a54d)

Co-authored-by: Serhiy Storchaka <[email protected]>
@miss-islington-app
Copy link

Sorry, @serhiy-storchaka, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 759a54d28ffe7eac8c23917f5d3dfad8309856be 3.12

@bedevere-app
Copy link

bedevere-app bot commented Oct 22, 2024

GH-125834 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Oct 22, 2024
serhiy-storchaka added a commit that referenced this pull request Oct 22, 2024
) (GH-125834)

* The parser no longer changes temporarily during parsing.
* Default values are not processed twice.
* Required mutually exclusive groups containing positional arguments are
  now supported.
* The missing arguments report now includes the names of all required
  optional and positional arguments.
* Unknown options can be intermixed with positional arguments in
  parse_known_intermixed_args().
(cherry picked from commit 759a54d)

Co-authored-by: Serhiy Storchaka <[email protected]>
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request Oct 22, 2024
…ythonGH-125356)

* The parser no longer changes temporarily during parsing.
* Default values are not processed twice.
* Required mutually exclusive groups containing positional arguments are
  now supported.
* The missing arguments report now includes the names of all required
  optional and positional arguments.
* Unknown options can be intermixed with positional arguments in
  parse_known_intermixed_args().
(cherry picked from commit 759a54d)

Co-authored-by: Serhiy Storchaka <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Oct 22, 2024

GH-125839 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Oct 22, 2024
serhiy-storchaka added a commit that referenced this pull request Oct 22, 2024
) (GH-125839)

* The parser no longer changes temporarily during parsing.
* Default values are not processed twice.
* Required mutually exclusive groups containing positional arguments are
  now supported.
* The missing arguments report now includes the names of all required
  optional and positional arguments.
* Unknown options can be intermixed with positional arguments in
  parse_known_intermixed_args().

(cherry picked from commit 759a54d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rewrite parse_intermixed_args() in argparse
1 participant