Skip to content

Commit

Permalink
pythongh-126068: Fix exceptions in the argparse module
Browse files Browse the repository at this point in the history
* Only error messages for ArgumentError and ArgumentTypeError are now
  translated.
* ArgumentError is now only used for command line errors, not for logical
  errors in the program.
  • Loading branch information
serhiy-storchaka committed Oct 28, 2024
1 parent 6870eb3 commit 42b91d3
Show file tree
Hide file tree
Showing 4 changed files with 46 additions and 42 deletions.
35 changes: 15 additions & 20 deletions Lib/argparse.py
Original file line number Diff line number Diff line change
Expand Up @@ -846,7 +846,7 @@ def format_usage(self):
return self.option_strings[0]

def __call__(self, parser, namespace, values, option_string=None):
raise NotImplementedError(_('.__call__() not defined'))
raise NotImplementedError('.__call__() not defined')


class BooleanOptionalAction(Action):
Expand Down Expand Up @@ -1172,11 +1172,10 @@ def add_parser(self, name, *, deprecated=False, **kwargs):
aliases = kwargs.pop('aliases', ())

if name in self._name_parser_map:
raise ArgumentError(self, _('conflicting subparser: %s') % name)
raise ValueError(f'conflicting subparser: {name}')
for alias in aliases:
if alias in self._name_parser_map:
raise ArgumentError(
self, _('conflicting subparser alias: %s') % alias)
raise ValueError(f'conflicting subparser alias: {alias}')

# create a pseudo-action to hold the choice help
if 'help' in kwargs:
Expand Down Expand Up @@ -1518,8 +1517,8 @@ def _add_container_actions(self, container):
if group.title in title_group_map:
# This branch could happen if a derived class added
# groups with duplicated titles in __init__
msg = _('cannot merge actions - two groups are named %r')
raise ValueError(msg % (group.title))
msg = f'cannot merge actions - two groups are named {group.title!r}'
raise ValueError(msg)
title_group_map[group.title] = group

# map each action to its group
Expand Down Expand Up @@ -1560,7 +1559,7 @@ def _add_container_actions(self, container):
def _get_positional_kwargs(self, dest, **kwargs):
# make sure required is not specified
if 'required' in kwargs:
msg = _("'required' is an invalid argument for positionals")
msg = "'required' is an invalid argument for positionals"
raise TypeError(msg)

# mark positional arguments as required if at least one is
Expand All @@ -1581,11 +1580,8 @@ def _get_optional_kwargs(self, *args, **kwargs):
for option_string in args:
# error on strings that don't start with an appropriate prefix
if not option_string[0] in self.prefix_chars:
args = {'option': option_string,
'prefix_chars': self.prefix_chars}
msg = _('invalid option string %(option)r: '
'must start with a character %(prefix_chars)r')
raise ValueError(msg % args)
raise ValueError(f'invalid option string {option_string!r}: '
f'must start with a character {self.prefix_chars!r}')

# strings starting with two prefix characters are long options
option_strings.append(option_string)
Expand All @@ -1601,8 +1597,8 @@ def _get_optional_kwargs(self, *args, **kwargs):
dest_option_string = option_strings[0]
dest = dest_option_string.lstrip(self.prefix_chars)
if not dest:
msg = _('dest= is required for options like %r')
raise ValueError(msg % option_string)
msg = f'dest= is required for options like {option_string!r}'
raise ValueError(msg)
dest = dest.replace('-', '_')

# return the updated keyword arguments
Expand All @@ -1618,8 +1614,8 @@ def _get_handler(self):
try:
return getattr(self, handler_func_name)
except AttributeError:
msg = _('invalid conflict_resolution value: %r')
raise ValueError(msg % self.conflict_handler)
msg = f'invalid conflict_resolution value: {self.conflict_handler!r}'
raise ValueError(msg)

def _check_conflict(self, action):

Expand Down Expand Up @@ -1727,7 +1723,7 @@ def __init__(self, container, required=False):

def _add_action(self, action):
if action.required:
msg = _('mutually exclusive arguments must be optional')
msg = 'mutually exclusive arguments must be optional'
raise ValueError(msg)
action = self._container._add_action(action)
self._group_actions.append(action)
Expand Down Expand Up @@ -1871,7 +1867,7 @@ def _get_kwargs(self):
# ==================================
def add_subparsers(self, **kwargs):
if self._subparsers is not None:
raise ArgumentError(None, _('cannot have multiple subparser arguments'))
raise ValueError('cannot have multiple subparser arguments')

# add the parser class to the arguments if it's not present
kwargs.setdefault('parser_class', type(self))
Expand Down Expand Up @@ -2565,8 +2561,7 @@ def _get_values(self, action, arg_strings):
def _get_value(self, action, arg_string):
type_func = self._registry_get('type', action.type, action.type)
if not callable(type_func):
msg = _('%r is not callable')
raise ArgumentError(action, msg % type_func)
raise RuntimeError('%r is not callable' % (type_func,))

# convert the value to the appropriate type
try:
Expand Down
39 changes: 28 additions & 11 deletions Lib/test/test_argparse.py
Original file line number Diff line number Diff line change
Expand Up @@ -2377,11 +2377,24 @@ def test_invalid_type(self):
self.assertRaises(NotImplementedError, parser.parse_args, ['--foo', 'bar'])

def test_modified_invalid_action(self):
parser = ErrorRaisingArgumentParser()
parser = argparse.ArgumentParser(exit_on_error=False)
action = parser.add_argument('--foo')
# Someone got crazy and did this
action.type = 1
self.assertRaises(ArgumentParserError, parser.parse_args, ['--foo', 'bar'])
self.assertRaisesRegex(RuntimeError, '1 is not callable',
parser.parse_args, ['--foo', 'bar'])
action.type = ()
self.assertRaisesRegex(RuntimeError, r'\(\) is not callable',
parser.parse_args, ['--foo', 'bar'])
# It is impossible to distinguish a TypeError raised due to mismatch
# of the required function arguments from a TypeError raised for wrong
# argument value, and it is not worth to use heavy inspect machinery
# which does not work in all cases anyway.
# So we get a dumb ArgumentError for such logical error.
action.type = pow
self.assertRaisesRegex(argparse.ArgumentError,
"argument --foo: invalid pow value: 'bar'",
parser.parse_args, ['--foo', 'bar'])


# ================
Expand Down Expand Up @@ -2418,7 +2431,7 @@ def _get_parser(self, subparser_help=False, prefix_chars=None,
else:
subparsers_kwargs['help'] = 'command help'
subparsers = parser.add_subparsers(**subparsers_kwargs)
self.assertRaisesRegex(argparse.ArgumentError,
self.assertRaisesRegex(ValueError,
'cannot have multiple subparser arguments',
parser.add_subparsers)

Expand Down Expand Up @@ -5733,14 +5746,18 @@ def test_subparser_conflict(self):
parser = argparse.ArgumentParser()
sp = parser.add_subparsers()
sp.add_parser('fullname', aliases=['alias'])
self.assertRaises(argparse.ArgumentError,
sp.add_parser, 'fullname')
self.assertRaises(argparse.ArgumentError,
sp.add_parser, 'alias')
self.assertRaises(argparse.ArgumentError,
sp.add_parser, 'other', aliases=['fullname'])
self.assertRaises(argparse.ArgumentError,
sp.add_parser, 'other', aliases=['alias'])
self.assertRaisesRegex(ValueError,
'conflicting subparser: fullname',
sp.add_parser, 'fullname')
self.assertRaisesRegex(ValueError,
'conflicting subparser: alias',
sp.add_parser, 'alias')
self.assertRaisesRegex(ValueError,
'conflicting subparser alias: fullname',
sp.add_parser, 'other', aliases=['fullname'])
self.assertRaisesRegex(ValueError,
'conflicting subparser alias: alias',
sp.add_parser, 'other', aliases=['alias'])


# =============================
Expand Down
11 changes: 0 additions & 11 deletions Lib/test/translationdata/argparse/msgids.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2,30 +2,19 @@
%(heading)s:
%(prog)s: error: %(message)s\n
%(prog)s: warning: %(message)s\n
%r is not callable
'required' is an invalid argument for positionals
.__call__() not defined
ambiguous option: %(option)s could match %(matches)s
argument "-" with mode %r
argument %(argument_name)s: %(message)s
argument '%(argument_name)s' is deprecated
can't open '%(filename)s': %(error)s
cannot have multiple subparser arguments
cannot merge actions - two groups are named %r
command '%(parser_name)s' is deprecated
conflicting subparser alias: %s
conflicting subparser: %s
dest= is required for options like %r
expected at least one argument
expected at most one argument
expected one argument
ignored explicit argument %r
invalid %(type)s value: %(value)r
invalid choice: %(value)r (choose from %(choices)s)
invalid choice: %(value)r, maybe you meant %(closest)r? (choose from %(choices)s)
invalid conflict_resolution value: %r
invalid option string %(option)r: must start with a character %(prefix_chars)r
mutually exclusive arguments must be optional
not allowed with argument %s
one of the arguments %s is required
option '%(option)s' is deprecated
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Fix exceptions in the :mod:`argparse` module. Only error messages for
ArgumentError and ArgumentTypeError are now translated. ArgumentError is now
only used for command line errors, not for logical errors in the program.

0 comments on commit 42b91d3

Please sign in to comment.