Skip to content

Commit

Permalink
pythongh-126068: Fix exceptions in the argparse module (pythonGH-126069)
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.
* TypeError is now raised instead of ValueError for some logical errors.
  • Loading branch information
serhiy-storchaka authored Oct 30, 2024
1 parent 1f16df4 commit cc9a183
Show file tree
Hide file tree
Showing 4 changed files with 72 additions and 58 deletions.
48 changes: 22 additions & 26 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 @@ -1430,8 +1429,8 @@ def add_argument(self, *args, **kwargs):
chars = self.prefix_chars
if not args or len(args) == 1 and args[0][0] not in chars:
if args and 'dest' in kwargs:
raise ValueError('dest supplied twice for positional argument,'
' did you mean metavar?')
raise TypeError('dest supplied twice for positional argument,'
' did you mean metavar?')
kwargs = self._get_positional_kwargs(*args, **kwargs)

# otherwise, we're adding an optional argument
Expand All @@ -1450,7 +1449,7 @@ def add_argument(self, *args, **kwargs):
action_name = kwargs.get('action')
action_class = self._pop_action_class(kwargs)
if not callable(action_class):
raise ValueError('unknown action "%s"' % (action_class,))
raise ValueError('unknown action {action_class!r}')
action = action_class(**kwargs)

# raise an error if action for positional argument does not
Expand All @@ -1461,11 +1460,11 @@ def add_argument(self, *args, **kwargs):
# raise an error if the action type is not callable
type_func = self._registry_get('type', action.type, action.type)
if not callable(type_func):
raise ValueError('%r is not callable' % (type_func,))
raise TypeError(f'{type_func!r} is not callable')

if type_func is FileType:
raise ValueError('%r is a FileType class object, instance of it'
' must be passed' % (type_func,))
raise TypeError(f'{type_func!r} is a FileType class object, '
f'instance of it must be passed')

# raise an error if the metavar does not match the type
if hasattr(self, "_get_formatter"):
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,9 @@ 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 +1598,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 TypeError(msg)
dest = dest.replace('-', '_')

# return the updated keyword arguments
Expand All @@ -1618,8 +1615,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 +1724,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 +1868,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 +2562,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 TypeError(f'{type_func!r} is not callable')

# convert the value to the appropriate type
try:
Expand Down
66 changes: 45 additions & 21 deletions Lib/test/test_argparse.py
Original file line number Diff line number Diff line change
Expand Up @@ -2055,7 +2055,7 @@ class TestFileTypeMissingInitialization(TestCase):

def test(self):
parser = argparse.ArgumentParser()
with self.assertRaises(ValueError) as cm:
with self.assertRaises(TypeError) as cm:
parser.add_argument('-x', type=argparse.FileType)

self.assertEqual(
Expand Down 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(TypeError, '1 is not callable',
parser.parse_args, ['--foo', 'bar'])
action.type = ()
self.assertRaisesRegex(TypeError, r'\(\) is not callable',
parser.parse_args, ['--foo', 'bar'])
# It is impossible to distinguish a TypeError raised due to a mismatch
# of the required function arguments from a TypeError raised for an incorrect
# argument value, and using the heavy inspection machinery is not worthwhile
# as it does not reliably work in all cases.
# Therefore, a generic ArgumentError is raised to handle this 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 @@ -5534,20 +5547,27 @@ def test_missing_destination(self):
self.assertTypeError(action=action)

def test_invalid_option_strings(self):
self.assertValueError('--')
self.assertValueError('---')
self.assertTypeError('-', errmsg='dest= is required')
self.assertTypeError('--', errmsg='dest= is required')
self.assertTypeError('---', errmsg='dest= is required')

def test_invalid_prefix(self):
self.assertValueError('--foo', '+foo')
self.assertValueError('--foo', '+foo',
errmsg='must start with a character')

def test_invalid_type(self):
self.assertValueError('--foo', type='int')
self.assertValueError('--foo', type=(int, float))
self.assertTypeError('--foo', type='int',
errmsg="'int' is not callable")
self.assertTypeError('--foo', type=(int, float),
errmsg='is not callable')

def test_invalid_action(self):
self.assertValueError('-x', action='foo')
self.assertValueError('foo', action='baz')
self.assertValueError('--foo', action=('store', 'append'))
self.assertValueError('-x', action='foo',
errmsg='unknown action')
self.assertValueError('foo', action='baz',
errmsg='unknown action')
self.assertValueError('--foo', action=('store', 'append'),
errmsg='unknown action')
self.assertValueError('--foo', action="store-true",
errmsg='unknown action')

Expand All @@ -5562,7 +5582,7 @@ def test_invalid_help(self):
def test_multiple_dest(self):
parser = argparse.ArgumentParser()
parser.add_argument(dest='foo')
with self.assertRaises(ValueError) as cm:
with self.assertRaises(TypeError) as cm:
parser.add_argument('bar', dest='baz')
self.assertIn('dest supplied twice for positional argument,'
' did you mean metavar?',
Expand Down Expand Up @@ -5733,14 +5753,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,5 @@
Fix exceptions in the :mod:`argparse` module so that 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. TypeError is now raised instead of ValueError for
some logical errors.

0 comments on commit cc9a183

Please sign in to comment.