From cc9a1839930d30450b12c460eee58137e3a3d6f1 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Wed, 30 Oct 2024 18:14:27 +0200 Subject: gh-126068: Fix exceptions in the argparse module (GH-126069) * 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. --- Lib/argparse.py | 48 ++++++++-------- Lib/test/test_argparse.py | 66 +++++++++++++++------- Lib/test/translationdata/argparse/msgids.txt | 11 ---- .../2024-10-28-11-33-59.gh-issue-126068.Pdznm_.rst | 5 ++ 4 files changed, 72 insertions(+), 58 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2024-10-28-11-33-59.gh-issue-126068.Pdznm_.rst diff --git a/Lib/argparse.py b/Lib/argparse.py index 9746173..072cd5e 100644 --- a/Lib/argparse.py +++ b/Lib/argparse.py @@ -846,7 +846,7 @@ class Action(_AttributeHolder): 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): @@ -1172,11 +1172,10 @@ class _SubParsersAction(Action): 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: @@ -1430,8 +1429,8 @@ class _ActionsContainer(object): 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 @@ -1450,7 +1449,7 @@ class _ActionsContainer(object): 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 @@ -1461,11 +1460,11 @@ class _ActionsContainer(object): # 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"): @@ -1518,8 +1517,8 @@ class _ActionsContainer(object): 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 @@ -1560,7 +1559,7 @@ class _ActionsContainer(object): 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 @@ -1581,11 +1580,9 @@ class _ActionsContainer(object): 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) @@ -1601,8 +1598,8 @@ class _ActionsContainer(object): 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 @@ -1618,8 +1615,8 @@ class _ActionsContainer(object): 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): @@ -1727,7 +1724,7 @@ class _MutuallyExclusiveGroup(_ArgumentGroup): 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) @@ -1871,7 +1868,7 @@ class ArgumentParser(_AttributeHolder, _ActionsContainer): # ================================== 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)) @@ -2565,8 +2562,7 @@ class ArgumentParser(_AttributeHolder, _ActionsContainer): 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: diff --git a/Lib/test/test_argparse.py b/Lib/test/test_argparse.py index 427e6bc..ba98765 100644 --- a/Lib/test/test_argparse.py +++ b/Lib/test/test_argparse.py @@ -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( @@ -2377,11 +2377,24 @@ class TestInvalidAction(TestCase): 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']) # ================ @@ -2418,7 +2431,7 @@ class TestAddSubparsers(TestCase): 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) @@ -5534,20 +5547,27 @@ class TestInvalidArgumentConstructors(TestCase): 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') @@ -5562,7 +5582,7 @@ class TestInvalidArgumentConstructors(TestCase): 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?', @@ -5733,14 +5753,18 @@ class TestConflictHandling(TestCase): 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']) # ============================= diff --git a/Lib/test/translationdata/argparse/msgids.txt b/Lib/test/translationdata/argparse/msgids.txt index f4d0a05..2b01290 100644 --- a/Lib/test/translationdata/argparse/msgids.txt +++ b/Lib/test/translationdata/argparse/msgids.txt @@ -2,20 +2,12 @@ %(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 @@ -23,9 +15,6 @@ 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 diff --git a/Misc/NEWS.d/next/Library/2024-10-28-11-33-59.gh-issue-126068.Pdznm_.rst b/Misc/NEWS.d/next/Library/2024-10-28-11-33-59.gh-issue-126068.Pdznm_.rst new file mode 100644 index 0000000..a0faf61 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2024-10-28-11-33-59.gh-issue-126068.Pdznm_.rst @@ -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. -- cgit v0.12