Skip to content

gh-125355: Rewrite parse_intermixed_args() in argparse #125356

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Oct 22, 2024
Merged

gh-125355: Rewrite parse_intermixed_args() in argparse #125356

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
342eb90
gh-125355: Rewrite parse_intermixed_args() in argparse
serhiy-storchaka Oct 12, 2024
a693da5
Enable support of required mutually exclusive group with positional.
serhiy-storchaka Oct 12, 2024
e305e86
Merge branch 'main' into argparse-parse-intermixed-args
serhiy-storchaka Oct 21, 2024
dbc8a86
Add a NEWS entry.
serhiy-storchaka Oct 22, 2024
c538416
Merge branch 'main' into argparse-parse-intermixed-args
serhiy-storchaka Oct 22, 2024
26840a8
Merge remote-tracking branch 'refs/remotes/origin/argparse-parse-inte…
serhiy-storchaka Oct 22, 2024
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
94 changes: 34 additions & 60 deletions Lib/argparse.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1905,6 +1905,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 @@ -1931,18 +1934,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 @@ -2033,6 +2036,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 @@ -2068,6 +2072,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 @@ -2146,6 +2151,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 @@ -2159,7 +2165,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 @@ -2175,16 +2181,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 @@ -2450,10 +2475,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 @@ -2467,54 +2488,7 @@ def parse_known_intermixed_args(self, args=None, namespace=None):
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
24 changes: 15 additions & 9 deletions Lib/test/test_argparse.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6277,12 +6277,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 Down Expand Up @@ -6327,11 +6338,6 @@ 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 @@ -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.

self.assertRegex(msg, 'req_opt')

# ==========================
Expand Down
Loading