From 1cb706c94b792241bb1b5b1c3eb415932dba8ed9 Mon Sep 17 00:00:00 2001 From: Tian Gao Date: Sat, 15 Apr 2023 16:12:14 -0700 Subject: [PATCH 01/14] Add coverage tests for argparse --- Lib/argparse.py | 27 ++++++++++++++------- Lib/test/test_argparse.py | 51 ++++++++++++++++++++++++++++++++++++--- 2 files changed, 66 insertions(+), 12 deletions(-) diff --git a/Lib/argparse.py b/Lib/argparse.py index a819d2650e85f0..5725f682074d5c 100644 --- a/Lib/argparse.py +++ b/Lib/argparse.py @@ -713,6 +713,10 @@ def _get_help_string(self, action): """ help = action.help if help is None: + # This ensures help would always be a string in an obvious way + # to make sure the contain check below works. + # However, in practice, the only way to access this code through + # normal operation has a checker for help being None already help = '' if '%(default)' not in help: @@ -1124,7 +1128,7 @@ class _VersionAction(Action): def __init__(self, option_strings, - version=None, + version, dest=SUPPRESS, default=SUPPRESS, help="show program's version number and exit"): @@ -1138,8 +1142,6 @@ def __init__(self, def __call__(self, parser, namespace, values, option_string=None): version = self.version - if version is None: - version = parser.version formatter = parser._get_formatter() formatter.add_text(version) parser._print_message(formatter.format_help(), _sys.stdout) @@ -1506,6 +1508,8 @@ def _add_container_actions(self, container): title_group_map = {} for group in self._action_groups: if group.title in title_group_map: + # This is practically impossible unless a derived class adds + # groups with duplicated titles in __init__. msg = _('cannot merge actions - two groups are named %r') raise ValueError(msg % (group.title)) title_group_map[group.title] = group @@ -1700,6 +1704,13 @@ def _add_action(self, action): return action def _remove_action(self, action): + # Currently this method will never be used because the only way to + # trigger _remove_action() method is from _handle_conflict_resolve + # and _MutuallyExclusiveGroup has to be an action container. + # However, the _add_action() method puts the action under the container + # of _MutuallyExclusiveGroup, the _remove_action() of + # _MutuallyExclusiveGroup will never be called - only the container's + # _remove_action() will be called. self._container._remove_action(action) self._group_actions.remove(action) @@ -1789,13 +1800,11 @@ def identity(string): # add parent arguments and defaults for parent in parents: + if not isinstance(parent, ArgumentParser): + raise TypeError('parents must be a list of ArgumentParser') self._add_container_actions(parent) - try: - defaults = parent._defaults - except AttributeError: - pass - else: - self._defaults.update(defaults) + defaults = parent._defaults + self._defaults.update(defaults) # ======================= # Pretty __repr__ methods diff --git a/Lib/test/test_argparse.py b/Lib/test/test_argparse.py index 861da2326d1214..bdad8318641365 100644 --- a/Lib/test/test_argparse.py +++ b/Lib/test/test_argparse.py @@ -13,6 +13,7 @@ import argparse import warnings +from contextlib import redirect_stderr from test.support import os_helper from unittest import mock @@ -1825,6 +1826,10 @@ def test_open_args(self): type('foo') m.assert_called_with('foo', *args) + def test_invalid_file_type(self): + with self.assertRaises(ValueError): + argparse.FileType('b')('-test') + class TestFileTypeMissingInitialization(TestCase): """ @@ -2018,6 +2023,27 @@ class TestActionExtend(ParserTestCase): ('--foo f1 --foo f2 f3 f4', NS(foo=['f1', 'f2', 'f3', 'f4'])), ] + +class TestInvalidAction(TestCase): + """Test invalid user defined Action""" + + class ActionWithoutCall(argparse.Action): + pass + + def test_invalid_type(self): + parser = argparse.ArgumentParser() + + parser.add_argument('--foo', action=self.ActionWithoutCall) + self.assertRaises(NotImplementedError, parser.parse_args, ['--foo', 'bar']) + + def test_modified_invalid_action(self): + parser = ErrorRaisingArgumentParser() + action = parser.add_argument('--foo') + # Someone got crazy and did this + action.type = 1 + self.assertRaises(ArgumentParserError, parser.parse_args, ['--foo', 'bar']) + + # ================ # Subparsers tests # ================ @@ -2653,6 +2679,9 @@ def test_groups_parents(self): -x X '''.format(progname, ' ' if progname else '' ))) + def test_wrong_type_parents(self): + self.assertRaises(TypeError, ErrorRaisingArgumentParser, parents=[1]) + # ============================== # Mutually exclusive group tests # ============================== @@ -4669,6 +4698,9 @@ def test_invalid_option_strings(self): self.assertValueError('--') self.assertValueError('---') + def test_invalid_prefix(self): + self.assertValueError('--foo', '+foo') + def test_invalid_type(self): self.assertValueError('--foo', type='int') self.assertValueError('--foo', type=(int, float)) @@ -4733,6 +4765,9 @@ def test_parsers_action_missing_params(self): self.assertTypeError('command', action='parsers', parser_class=argparse.ArgumentParser) + def test_version_missing_params(self): + self.assertTypeError('command', action='version') + def test_required_positional(self): self.assertTypeError('foo', required=True) @@ -4904,10 +4939,7 @@ def test_no_help(self): def test_alternate_help_version(self): parser = ErrorRaisingArgumentParser() parser.add_argument('-x', action='help') - parser.add_argument('-y', action='version') self.assertPrintHelpExit(parser, '-x') - self.assertArgumentParserError(parser, '-v') - self.assertArgumentParserError(parser, '--version') self.assertRaises(AttributeError, getattr, parser, 'format_version') def test_help_version_extra_arguments(self): @@ -5326,6 +5358,19 @@ def test_exclusive_incompatible(self): self.assertRaises(TypeError, parser.parse_intermixed_args, []) 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') + parser.parse_args() + with io.StringIO() as buf, redirect_stderr(buf): + parser.parse_intermixed_args(['hello', '--foo']) + output = buf.getvalue() + self.assertIn("UserWarning", output) + class TestIntermixedMessageContentError(TestCase): # case where Intermixed gives different error message # error is raised by 1st parsing step From 78a4ffe4c9ad0530d69614c15e185a3038ca7100 Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Sat, 15 Apr 2023 23:26:17 +0000 Subject: [PATCH 02/14] =?UTF-8?q?=F0=9F=93=9C=F0=9F=A4=96=20Added=20by=20b?= =?UTF-8?q?lurb=5Fit.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../next/Library/2023-04-15-23-26-16.gh-issue-103558.w9OzK4.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Library/2023-04-15-23-26-16.gh-issue-103558.w9OzK4.rst diff --git a/Misc/NEWS.d/next/Library/2023-04-15-23-26-16.gh-issue-103558.w9OzK4.rst b/Misc/NEWS.d/next/Library/2023-04-15-23-26-16.gh-issue-103558.w9OzK4.rst new file mode 100644 index 00000000000000..7512c3b93212b2 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2023-04-15-23-26-16.gh-issue-103558.w9OzK4.rst @@ -0,0 +1 @@ +Fixed `version` and `parent` argument validation mechanism of :mod:`argparse`. Improved test coverage. From 2fe75c6d4aae186dc4b15370a49aeb44ed5ca24d Mon Sep 17 00:00:00 2001 From: Tian Gao Date: Sat, 15 Apr 2023 17:55:09 -0700 Subject: [PATCH 03/14] Remove a wrong & useless line --- Lib/test/test_argparse.py | 1 - 1 file changed, 1 deletion(-) diff --git a/Lib/test/test_argparse.py b/Lib/test/test_argparse.py index bdad8318641365..4ba6b1495a32b9 100644 --- a/Lib/test/test_argparse.py +++ b/Lib/test/test_argparse.py @@ -5365,7 +5365,6 @@ def test_invalid_args(self): parser = ErrorRaisingArgumentParser(prog='PROG') parser.add_argument('--foo', nargs="*") parser.add_argument('foo') - parser.parse_args() with io.StringIO() as buf, redirect_stderr(buf): parser.parse_intermixed_args(['hello', '--foo']) output = buf.getvalue() From 5a6a9c34ede9b8da4579d45aeb2083b2180a0723 Mon Sep 17 00:00:00 2001 From: Tian Gao Date: Sat, 15 Apr 2023 17:57:52 -0700 Subject: [PATCH 04/14] Update 2023-04-15-23-26-16.gh-issue-103558.w9OzK4.rst --- .../next/Library/2023-04-15-23-26-16.gh-issue-103558.w9OzK4.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Library/2023-04-15-23-26-16.gh-issue-103558.w9OzK4.rst b/Misc/NEWS.d/next/Library/2023-04-15-23-26-16.gh-issue-103558.w9OzK4.rst index 7512c3b93212b2..abadc3f525aca3 100644 --- a/Misc/NEWS.d/next/Library/2023-04-15-23-26-16.gh-issue-103558.w9OzK4.rst +++ b/Misc/NEWS.d/next/Library/2023-04-15-23-26-16.gh-issue-103558.w9OzK4.rst @@ -1 +1 @@ -Fixed `version` and `parent` argument validation mechanism of :mod:`argparse`. Improved test coverage. +Fixed `version` and `parent` argument validation mechanism of :mod:``argparse``. Improved test coverage. From 3423900cf32957875ca10ed4dc0387cd952bea0a Mon Sep 17 00:00:00 2001 From: Tian Gao Date: Sat, 15 Apr 2023 17:58:07 -0700 Subject: [PATCH 05/14] Update 2023-04-15-23-26-16.gh-issue-103558.w9OzK4.rst --- .../next/Library/2023-04-15-23-26-16.gh-issue-103558.w9OzK4.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Library/2023-04-15-23-26-16.gh-issue-103558.w9OzK4.rst b/Misc/NEWS.d/next/Library/2023-04-15-23-26-16.gh-issue-103558.w9OzK4.rst index abadc3f525aca3..80d90a593481a4 100644 --- a/Misc/NEWS.d/next/Library/2023-04-15-23-26-16.gh-issue-103558.w9OzK4.rst +++ b/Misc/NEWS.d/next/Library/2023-04-15-23-26-16.gh-issue-103558.w9OzK4.rst @@ -1 +1 @@ -Fixed `version` and `parent` argument validation mechanism of :mod:``argparse``. Improved test coverage. +Fixed ``version`` and ``parent`` argument validation mechanism of :mod:`argparse`. Improved test coverage. From 6d9224a7104a57862c141b752944fe7a435366da Mon Sep 17 00:00:00 2001 From: Tian Gao Date: Mon, 17 Apr 2023 17:52:16 -0700 Subject: [PATCH 06/14] Revert version changes --- Lib/argparse.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Lib/argparse.py b/Lib/argparse.py index 5725f682074d5c..32d0b114eae802 100644 --- a/Lib/argparse.py +++ b/Lib/argparse.py @@ -1128,7 +1128,7 @@ class _VersionAction(Action): def __init__(self, option_strings, - version, + version=None, dest=SUPPRESS, default=SUPPRESS, help="show program's version number and exit"): @@ -1142,6 +1142,8 @@ def __init__(self, def __call__(self, parser, namespace, values, option_string=None): version = self.version + if version is None: + version = parser.version formatter = parser._get_formatter() formatter.add_text(version) parser._print_message(formatter.format_help(), _sys.stdout) From fdb9929c7b26efdf348d9be257d288ca0fc4932f Mon Sep 17 00:00:00 2001 From: Tian Gao Date: Mon, 17 Apr 2023 17:54:06 -0700 Subject: [PATCH 07/14] Use captured_stderr --- Lib/test/test_argparse.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/Lib/test/test_argparse.py b/Lib/test/test_argparse.py index 4ba6b1495a32b9..fad8e05ef178e7 100644 --- a/Lib/test/test_argparse.py +++ b/Lib/test/test_argparse.py @@ -13,8 +13,7 @@ import argparse import warnings -from contextlib import redirect_stderr -from test.support import os_helper +from test.support import os_helper, captured_stderr from unittest import mock @@ -5365,10 +5364,9 @@ def test_invalid_args(self): parser = ErrorRaisingArgumentParser(prog='PROG') parser.add_argument('--foo', nargs="*") parser.add_argument('foo') - with io.StringIO() as buf, redirect_stderr(buf): + with captured_stderr() as stderr: parser.parse_intermixed_args(['hello', '--foo']) - output = buf.getvalue() - self.assertIn("UserWarning", output) + self.assertIn("UserWarning", stderr.getvalue()) class TestIntermixedMessageContentError(TestCase): # case where Intermixed gives different error message From 2460a79cc8f294f657858da1a92d8e682881c969 Mon Sep 17 00:00:00 2001 From: Tian Gao Date: Mon, 17 Apr 2023 17:55:34 -0700 Subject: [PATCH 08/14] Update 2023-04-15-23-26-16.gh-issue-103558.w9OzK4.rst --- .../next/Library/2023-04-15-23-26-16.gh-issue-103558.w9OzK4.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Library/2023-04-15-23-26-16.gh-issue-103558.w9OzK4.rst b/Misc/NEWS.d/next/Library/2023-04-15-23-26-16.gh-issue-103558.w9OzK4.rst index 80d90a593481a4..e62af647fcc96a 100644 --- a/Misc/NEWS.d/next/Library/2023-04-15-23-26-16.gh-issue-103558.w9OzK4.rst +++ b/Misc/NEWS.d/next/Library/2023-04-15-23-26-16.gh-issue-103558.w9OzK4.rst @@ -1 +1 @@ -Fixed ``version`` and ``parent`` argument validation mechanism of :mod:`argparse`. Improved test coverage. +Fixed ``parent`` argument validation mechanism of :mod:`argparse`. Improved test coverage. From 7b53ac9997a170f3de849ec6f685b7bdf17ff240 Mon Sep 17 00:00:00 2001 From: Tian Gao Date: Mon, 17 Apr 2023 19:05:15 -0700 Subject: [PATCH 09/14] Add positional append/extend tests --- Lib/test/test_argparse.py | 43 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/Lib/test/test_argparse.py b/Lib/test/test_argparse.py index fad8e05ef178e7..8af21c1dc3cff7 100644 --- a/Lib/test/test_argparse.py +++ b/Lib/test/test_argparse.py @@ -1308,6 +1308,19 @@ class TestPositionalsActionAppend(ParserTestCase): ('a b c', NS(spam=['a', ['b', 'c']])), ] + +class TestPositionalsActionExtend(ParserTestCase): + """Test the 'extend' action""" + + argument_signatures = [ + Sig('spam', action='extend'), + Sig('spam', action='extend', nargs=2), + ] + failures = ['', '--foo', 'a', 'a b', 'a b c d'] + successes = [ + ('a b c', NS(spam=['a', 'b', 'c'])), + ] + # ======================================== # Combined optionals and positionals tests # ======================================== @@ -1345,6 +1358,36 @@ class TestOptionalsAlmostNumericAndPositionals(ParserTestCase): ] +class TestOptionalsAndPositionalsAppend(ParserTestCase): + """Tests negative number args when numeric options are present""" + + argument_signatures = [ + Sig('foo', nargs='*', action='append'), + Sig('--bar'), + ] + failures = ['-foo'] + successes = [ + ('a b', NS(foo=[['a', 'b']], bar=None)), + ('--bar a b', NS(foo=[['b']], bar='a')), + ('a b --bar c', NS(foo=[['a', 'b']], bar='c')), + ] + + +class TestOptionalsAndPositionalsExtend(ParserTestCase): + """Tests negative number args when numeric options are present""" + + argument_signatures = [ + Sig('foo', nargs='*', action='extend'), + Sig('--bar'), + ] + failures = ['-foo'] + successes = [ + ('a b', NS(foo=['a', 'b'], bar=None)), + ('--bar a b', NS(foo=['b'], bar='a')), + ('a b --bar c', NS(foo=['a', 'b'], bar='c')), + ] + + class TestEmptyAndSpaceContainingArguments(ParserTestCase): argument_signatures = [ From 4b2b1f656edfead3d76c98a41f7c80813cf88e68 Mon Sep 17 00:00:00 2001 From: hauntsaninja Date: Sun, 4 Jun 2023 23:29:51 -0700 Subject: [PATCH 10/14] this comment explains test coverage, but isn't useful in general --- Lib/argparse.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/Lib/argparse.py b/Lib/argparse.py index d0b9357299969f..b1494d75275dc2 100644 --- a/Lib/argparse.py +++ b/Lib/argparse.py @@ -714,10 +714,6 @@ def _get_help_string(self, action): """ help = action.help if help is None: - # This ensures help would always be a string in an obvious way - # to make sure the contain check below works. - # However, in practice, the only way to access this code through - # normal operation has a checker for help being None already help = '' if '%(default)' not in help: From c3db2b667b01c351c1c20a9355b07d8f1da0a114 Mon Sep 17 00:00:00 2001 From: hauntsaninja Date: Sun, 4 Jun 2023 23:30:12 -0700 Subject: [PATCH 11/14] "impossible" is a strong word :-) --- Lib/argparse.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/argparse.py b/Lib/argparse.py index b1494d75275dc2..708590ef1751ab 100644 --- a/Lib/argparse.py +++ b/Lib/argparse.py @@ -1528,8 +1528,8 @@ def _add_container_actions(self, container): title_group_map = {} for group in self._action_groups: if group.title in title_group_map: - # This is practically impossible unless a derived class adds - # groups with duplicated titles in __init__. + # 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)) title_group_map[group.title] = group From 81f2322b1dc4eb09978f845c0c193f4853f63953 Mon Sep 17 00:00:00 2001 From: hauntsaninja Date: Sun, 4 Jun 2023 23:37:37 -0700 Subject: [PATCH 12/14] remove comment that i don't understand the code well enough to verify --- Lib/argparse.py | 7 ------- 1 file changed, 7 deletions(-) diff --git a/Lib/argparse.py b/Lib/argparse.py index 708590ef1751ab..dfc98695f64e0a 100644 --- a/Lib/argparse.py +++ b/Lib/argparse.py @@ -1724,13 +1724,6 @@ def _add_action(self, action): return action def _remove_action(self, action): - # Currently this method will never be used because the only way to - # trigger _remove_action() method is from _handle_conflict_resolve - # and _MutuallyExclusiveGroup has to be an action container. - # However, the _add_action() method puts the action under the container - # of _MutuallyExclusiveGroup, the _remove_action() of - # _MutuallyExclusiveGroup will never be called - only the container's - # _remove_action() will be called. self._container._remove_action(action) self._group_actions.remove(action) From effbb7b3c96e9114f05088032eb0c49812cf867c Mon Sep 17 00:00:00 2001 From: hauntsaninja Date: Sun, 4 Jun 2023 23:39:17 -0700 Subject: [PATCH 13/14] remove incorrect docstrings --- Lib/test/test_argparse.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/Lib/test/test_argparse.py b/Lib/test/test_argparse.py index c8200194398dea..04f733e3ce9592 100644 --- a/Lib/test/test_argparse.py +++ b/Lib/test/test_argparse.py @@ -1433,8 +1433,6 @@ class TestOptionalsAlmostNumericAndPositionals(ParserTestCase): class TestOptionalsAndPositionalsAppend(ParserTestCase): - """Tests negative number args when numeric options are present""" - argument_signatures = [ Sig('foo', nargs='*', action='append'), Sig('--bar'), @@ -1448,8 +1446,6 @@ class TestOptionalsAndPositionalsAppend(ParserTestCase): class TestOptionalsAndPositionalsExtend(ParserTestCase): - """Tests negative number args when numeric options are present""" - argument_signatures = [ Sig('foo', nargs='*', action='extend'), Sig('--bar'), From 033d11268b82e7445ef8c1ece46af93622e03c25 Mon Sep 17 00:00:00 2001 From: hauntsaninja Date: Sun, 4 Jun 2023 23:43:02 -0700 Subject: [PATCH 14/14] restore tests --- Lib/test/test_argparse.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Lib/test/test_argparse.py b/Lib/test/test_argparse.py index 04f733e3ce9592..7c1f5d36999a3d 100644 --- a/Lib/test/test_argparse.py +++ b/Lib/test/test_argparse.py @@ -5051,7 +5051,10 @@ def test_no_help(self): def test_alternate_help_version(self): parser = ErrorRaisingArgumentParser() parser.add_argument('-x', action='help') + parser.add_argument('-y', action='version') self.assertPrintHelpExit(parser, '-x') + self.assertArgumentParserError(parser, '-v') + self.assertArgumentParserError(parser, '--version') self.assertRaises(AttributeError, getattr, parser, 'format_version') def test_help_version_extra_arguments(self):