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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 34 additions & 65 deletions Lib/argparse.py
Original file line number Diff line number Diff line change
Expand Up @@ -1924,6 +1924,9 @@ def parse_args(self, args=None, namespace=None):
return args

def parse_known_args(self, args=None, namespace=None):
return self._parse_known_args2(args, namespace, intermixed=False)

def _parse_known_args2(self, args, namespace, intermixed):
if args is None:
# args default to the system args
args = _sys.argv[1:]
Expand All @@ -1950,18 +1953,18 @@ def parse_known_args(self, args=None, namespace=None):
# parse the arguments and exit if there are any errors
if self.exit_on_error:
try:
namespace, args = self._parse_known_args(args, namespace)
namespace, args = self._parse_known_args(args, namespace, intermixed)
except ArgumentError as err:
self.error(str(err))
else:
namespace, args = self._parse_known_args(args, namespace)
namespace, args = self._parse_known_args(args, namespace, intermixed)

if hasattr(namespace, _UNRECOGNIZED_ARGS_ATTR):
args.extend(getattr(namespace, _UNRECOGNIZED_ARGS_ATTR))
delattr(namespace, _UNRECOGNIZED_ARGS_ATTR)
return namespace, args

def _parse_known_args(self, arg_strings, namespace):
def _parse_known_args(self, arg_strings, namespace, intermixed):
# replace arg strings that are file references
if self.fromfile_prefix_chars is not None:
arg_strings = self._read_args_from_files(arg_strings)
Expand Down Expand Up @@ -2052,6 +2055,7 @@ def consume_optional(start_index):
# if we found no optional action, skip it
if action is None:
extras.append(arg_strings[start_index])
extras_pattern.append('O')
return start_index + 1

# if there is an explicit argument, try to match the
Expand Down Expand Up @@ -2087,6 +2091,7 @@ def consume_optional(start_index):
sep = ''
else:
extras.append(char + explicit_arg)
extras_pattern.append('O')
stop = start_index + 1
break
# if the action expect exactly one argument, we've
Expand Down Expand Up @@ -2165,6 +2170,7 @@ def consume_positionals(start_index):
# consume Positionals and Optionals alternately, until we have
# passed the last option string
extras = []
extras_pattern = []
start_index = 0
if option_string_indices:
max_option_string_index = max(option_string_indices)
Expand All @@ -2178,7 +2184,7 @@ def consume_positionals(start_index):
if next_option_string_index in option_string_indices:
break
next_option_string_index += 1
if start_index != next_option_string_index:
if not intermixed and start_index != next_option_string_index:
positionals_end_index = consume_positionals(start_index)

# only try to parse the next optional if we didn't consume
Expand All @@ -2194,16 +2200,35 @@ def consume_positionals(start_index):
if start_index not in option_string_indices:
strings = arg_strings[start_index:next_option_string_index]
extras.extend(strings)
extras_pattern.extend(arg_strings_pattern[start_index:next_option_string_index])
start_index = next_option_string_index

# consume the next optional and any arguments for it
start_index = consume_optional(start_index)

# consume any positionals following the last Optional
stop_index = consume_positionals(start_index)
if not intermixed:
# consume any positionals following the last Optional
stop_index = consume_positionals(start_index)

# if we didn't consume all the argument strings, there were extras
extras.extend(arg_strings[stop_index:])
# if we didn't consume all the argument strings, there were extras
extras.extend(arg_strings[stop_index:])
else:
extras.extend(arg_strings[start_index:])
extras_pattern.extend(arg_strings_pattern[start_index:])
extras_pattern = ''.join(extras_pattern)
assert len(extras_pattern) == len(extras)
# consume all positionals
arg_strings = [s for s, c in zip(extras, extras_pattern) if c != 'O']
arg_strings_pattern = extras_pattern.replace('O', '')
stop_index = consume_positionals(0)
# leave unknown optionals and non-consumed positionals in extras
for i, c in enumerate(extras_pattern):
if not stop_index:
break
if c != 'O':
stop_index -= 1
extras[i] = None
extras = [s for s in extras if s is not None]

# make sure all required actions were present and also convert
# action defaults which were not given as arguments
Expand Down Expand Up @@ -2469,10 +2494,6 @@ def parse_known_intermixed_args(self, args=None, namespace=None):
# are then parsed. If the parser definition is incompatible with the
# intermixed assumptions (e.g. use of REMAINDER, subparsers) a
# TypeError is raised.
#
# positionals are 'deactivated' by setting nargs and default to
# SUPPRESS. This blocks the addition of that positional to the
# namespace

positionals = self._get_positional_actions()
a = [action for action in positionals
Expand All @@ -2481,59 +2502,7 @@ def parse_known_intermixed_args(self, args=None, namespace=None):
raise TypeError('parse_intermixed_args: positional arg'
' with nargs=%s'%a[0].nargs)

if [action.dest for group in self._mutually_exclusive_groups
for action in group._group_actions if action in positionals]:
raise TypeError('parse_intermixed_args: positional in'
' mutuallyExclusiveGroup')

try:
save_usage = self.usage
try:
if self.usage is None:
# capture the full usage for use in error messages
self.usage = self.format_usage()[7:]
for action in positionals:
# deactivate positionals
action.save_nargs = action.nargs
# action.nargs = 0
action.nargs = SUPPRESS
action.save_default = action.default
action.default = SUPPRESS
namespace, remaining_args = self.parse_known_args(args,
namespace)
for action in positionals:
# remove the empty positional values from namespace
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)
Comment on lines -2506 to -2510
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?

finally:
# restore nargs and usage before exiting
for action in positionals:
action.nargs = action.save_nargs
action.default = action.save_default
optionals = self._get_optional_actions()
try:
# parse positionals. optionals aren't normally required, but
# they could be, so make sure they aren't.
for action in optionals:
action.save_required = action.required
action.required = False
for group in self._mutually_exclusive_groups:
group.save_required = group.required
group.required = False
namespace, extras = self.parse_known_args(remaining_args,
namespace)
finally:
# restore parser values before exiting
for action in optionals:
action.required = action.save_required
for group in self._mutually_exclusive_groups:
group.required = group.save_required
finally:
self.usage = save_usage
return namespace, extras
return self._parse_known_args2(args, namespace, intermixed=True)

# ========================
# Value conversion methods
Expand Down
56 changes: 39 additions & 17 deletions Lib/test/test_argparse.py
Original file line number Diff line number Diff line change
Expand Up @@ -6412,12 +6412,23 @@ def test_basic(self):
# cannot parse the '1,2,3'
self.assertEqual(NS(bar='y', cmd='cmd', foo='x', rest=[1]), args)
self.assertEqual(["2", "3"], extras)
args, extras = parser.parse_known_intermixed_args(argv)
self.assertEqual(NS(bar='y', cmd='cmd', foo='x', rest=[1, 2, 3]), args)
self.assertEqual([], extras)

# unknown optionals go into extras
argv = 'cmd --foo x --error 1 2 --bar y 3'.split()
args, extras = parser.parse_known_intermixed_args(argv)
self.assertEqual(NS(bar='y', cmd='cmd', foo='x', rest=[1, 2, 3]), args)
self.assertEqual(['--error'], extras)
argv = 'cmd --foo x 1 --error 2 --bar y 3'.split()
args, extras = parser.parse_known_intermixed_args(argv)
# unknown optionals go into extras
self.assertEqual(NS(bar='y', cmd='cmd', foo='x', rest=[1]), args)
self.assertEqual(['--error', '2', '3'], extras)
Comment on lines -6419 to -6420
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.

self.assertEqual(NS(bar='y', cmd='cmd', foo='x', rest=[1, 2, 3]), args)
self.assertEqual(['--error'], extras)
argv = 'cmd --foo x 1 2 --error --bar y 3'.split()
args, extras = parser.parse_known_intermixed_args(argv)
self.assertEqual(NS(bar='y', cmd='cmd', foo='x', rest=[1, 2, 3]), args)
self.assertEqual(['--error'], extras)

# restores attributes that were temporarily changed
self.assertIsNone(parser.usage)
Expand All @@ -6436,37 +6447,48 @@ def test_remainder(self):
parser.parse_intermixed_args(argv)
self.assertRegex(str(cm.exception), r'\.\.\.')

def test_exclusive(self):
# mutually exclusive group; intermixed works fine
parser = ErrorRaisingArgumentParser(prog='PROG')
def test_required_exclusive(self):
# required mutually exclusive group; intermixed works fine
parser = argparse.ArgumentParser(prog='PROG', exit_on_error=False)
group = parser.add_mutually_exclusive_group(required=True)
group.add_argument('--foo', action='store_true', help='FOO')
group.add_argument('--spam', help='SPAM')
parser.add_argument('badger', nargs='*', default='X', help='BADGER')
args = parser.parse_intermixed_args('--foo 1 2'.split())
self.assertEqual(NS(badger=['1', '2'], foo=True, spam=None), args)
args = parser.parse_intermixed_args('1 --foo 2'.split())
self.assertEqual(NS(badger=['1', '2'], foo=True, spam=None), args)
self.assertRaises(ArgumentParserError, parser.parse_intermixed_args, '1 2'.split())
self.assertRaisesRegex(argparse.ArgumentError,
'one of the arguments --foo --spam is required',
parser.parse_intermixed_args, '1 2'.split())
self.assertEqual(group.required, True)

def test_exclusive_incompatible(self):
# mutually exclusive group including positional - fail
parser = ErrorRaisingArgumentParser(prog='PROG')
def test_required_exclusive_with_positional(self):
# required mutually exclusive group with positional argument
parser = argparse.ArgumentParser(prog='PROG', exit_on_error=False)
group = parser.add_mutually_exclusive_group(required=True)
group.add_argument('--foo', action='store_true', help='FOO')
group.add_argument('--spam', help='SPAM')
group.add_argument('badger', nargs='*', default='X', help='BADGER')
self.assertRaises(TypeError, parser.parse_intermixed_args, [])
args = parser.parse_intermixed_args(['--foo'])
self.assertEqual(NS(foo=True, spam=None, badger='X'), args)
args = parser.parse_intermixed_args(['a', 'b'])
self.assertEqual(NS(foo=False, spam=None, badger=['a', 'b']), args)
self.assertRaisesRegex(argparse.ArgumentError,
'one of the arguments --foo --spam badger is required',
parser.parse_intermixed_args, [])
self.assertRaisesRegex(argparse.ArgumentError,
'argument badger: not allowed with argument --foo',
parser.parse_intermixed_args, ['--foo', 'a', 'b'])
self.assertRaisesRegex(argparse.ArgumentError,
'argument badger: not allowed with argument --foo',
parser.parse_intermixed_args, ['a', '--foo', 'b'])
self.assertEqual(group.required, True)

def test_invalid_args(self):
parser = ErrorRaisingArgumentParser(prog='PROG')
self.assertRaises(ArgumentParserError, parser.parse_intermixed_args, ['a'])

parser = ErrorRaisingArgumentParser(prog='PROG')
parser.add_argument('--foo', nargs="*")
parser.add_argument('foo')
with self.assertWarns(UserWarning):
parser.parse_intermixed_args(['hello', '--foo'])
Comment on lines -6465 to -6469
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.


class TestIntermixedMessageContentError(TestCase):
# case where Intermixed gives different error message
Expand All @@ -6485,7 +6507,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.

self.assertRegex(msg, 'req_opt')

# ==========================
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
Fix several bugs in :meth:`argparse.ArgumentParser.parse_intermixed_args`.

* 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().
Loading