Skip to content

bpo-11874 argparse assertion failure #1826

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 3 commits into from
Jun 8, 2018
Merged

bpo-11874 argparse assertion failure #1826

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ab9cd7f
fix some incorrect tests that did not make 1-element tuples correctly
wimglenn May 26, 2017
d536c8f
bugfix and test for fragile asserts in argparse library code (see iss…
wimglenn May 26, 2017
0fc1cad
Addressed reviewer comments. Added the NEWS.d entry
wimglenn May 23, 2018
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
6 changes: 5 additions & 1 deletion Lib/argparse.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,11 @@ def _format_usage(self, usage, actions, groups, prefix):
if len(prefix) + len(usage) > text_width:

# break usage into wrappable parts
part_regexp = r'\(.*?\)+|\[.*?\]+|\S+'
part_regexp = (
r'\(.*?\)+(?=\s|$)|'
r'\[.*?\]+(?=\s|$)|'
r'\S+'
)
opt_usage = format(optionals, groups)
pos_usage = format(positionals, groups)
opt_parts = _re.findall(part_regexp, opt_usage)
Expand Down
42 changes: 33 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 @@ -4953,7 +4953,7 @@ def test_nargs_None_metavar_length0(self):
self.do_test_exception(nargs=None, metavar=tuple())

def test_nargs_None_metavar_length1(self):
self.do_test_no_exception(nargs=None, metavar=("1"))
self.do_test_no_exception(nargs=None, metavar=("1",))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear how these string -> tuple changes relate to the rest of the PR. They give the impression that tests needed to be changed in order to avoid test failures (which I don't believe to be the case).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's why I've isolated them in a separate commit - the tests were bogus, not testing what they claimed to be testing, and actually 4 of the assertions were opposite of what they should have been. Just cleaning up whilst I was touching this file anyway.


def test_nargs_None_metavar_length2(self):
self.do_test_exception(nargs=None, metavar=("1", "2"))
Expand All @@ -4970,7 +4970,7 @@ def test_nargs_optional_metavar_length0(self):
self.do_test_exception(nargs="?", metavar=tuple())

def test_nargs_optional_metavar_length1(self):
self.do_test_no_exception(nargs="?", metavar=("1"))
self.do_test_no_exception(nargs="?", metavar=("1",))

def test_nargs_optional_metavar_length2(self):
self.do_test_exception(nargs="?", metavar=("1", "2"))
Expand All @@ -4987,7 +4987,7 @@ def test_nargs_zeroormore_metavar_length0(self):
self.do_test_exception(nargs="*", metavar=tuple())

def test_nargs_zeroormore_metavar_length1(self):
self.do_test_no_exception(nargs="*", metavar=("1"))
self.do_test_exception(nargs="*", metavar=("1",))

def test_nargs_zeroormore_metavar_length2(self):
self.do_test_no_exception(nargs="*", metavar=("1", "2"))
Expand All @@ -5004,7 +5004,7 @@ def test_nargs_oneormore_metavar_length0(self):
self.do_test_exception(nargs="+", metavar=tuple())

def test_nargs_oneormore_metavar_length1(self):
self.do_test_no_exception(nargs="+", metavar=("1"))
self.do_test_exception(nargs="+", metavar=("1",))

def test_nargs_oneormore_metavar_length2(self):
self.do_test_no_exception(nargs="+", metavar=("1", "2"))
Expand All @@ -5021,7 +5021,7 @@ def test_nargs_remainder_metavar_length0(self):
self.do_test_no_exception(nargs="...", metavar=tuple())

def test_nargs_remainder_metavar_length1(self):
self.do_test_no_exception(nargs="...", metavar=("1"))
self.do_test_no_exception(nargs="...", metavar=("1",))

def test_nargs_remainder_metavar_length2(self):
self.do_test_no_exception(nargs="...", metavar=("1", "2"))
Expand All @@ -5038,7 +5038,7 @@ def test_nargs_parser_metavar_length0(self):
self.do_test_exception(nargs="A...", metavar=tuple())

def test_nargs_parser_metavar_length1(self):
self.do_test_no_exception(nargs="A...", metavar=("1"))
self.do_test_no_exception(nargs="A...", metavar=("1",))

def test_nargs_parser_metavar_length2(self):
self.do_test_exception(nargs="A...", metavar=("1", "2"))
Expand All @@ -5055,7 +5055,7 @@ def test_nargs_1_metavar_length0(self):
self.do_test_exception(nargs=1, metavar=tuple())

def test_nargs_1_metavar_length1(self):
self.do_test_no_exception(nargs=1, metavar=("1"))
self.do_test_no_exception(nargs=1, metavar=("1",))

def test_nargs_1_metavar_length2(self):
self.do_test_exception(nargs=1, metavar=("1", "2"))
Expand All @@ -5072,7 +5072,7 @@ def test_nargs_2_metavar_length0(self):
self.do_test_exception(nargs=2, metavar=tuple())

def test_nargs_2_metavar_length1(self):
self.do_test_no_exception(nargs=2, metavar=("1"))
self.do_test_exception(nargs=2, metavar=("1",))

def test_nargs_2_metavar_length2(self):
self.do_test_no_exception(nargs=2, metavar=("1", "2"))
Expand All @@ -5089,7 +5089,7 @@ def test_nargs_3_metavar_length0(self):
self.do_test_exception(nargs=3, metavar=tuple())

def test_nargs_3_metavar_length1(self):
self.do_test_no_exception(nargs=3, metavar=("1"))
self.do_test_exception(nargs=3, metavar=("1",))

def test_nargs_3_metavar_length2(self):
self.do_test_exception(nargs=3, metavar=("1", "2"))
Expand All @@ -5116,6 +5116,30 @@ def test_all_exports_everything_but_modules(self):
]
self.assertEqual(sorted(items), sorted(argparse.__all__))


class TestWrappingMetavar(TestCase):

def setUp(self):
self.parser = ErrorRaisingArgumentParser(
'this_is_spammy_prog_with_a_long_name_sorry_about_the_name'
)
# this metavar was triggering library assertion errors due to usage
# message formatting incorrectly splitting on the ] chars within
metavar = '<http[s]://example:1234>'
self.parser.add_argument('--proxy', metavar=metavar)

def test_help_with_metavar(self):
help_text = self.parser.format_help()
self.assertEqual(help_text, textwrap.dedent('''\
usage: this_is_spammy_prog_with_a_long_name_sorry_about_the_name
[-h] [--proxy <http[s]://example:1234>]

optional arguments:
-h, --help show this help message and exit
--proxy <http[s]://example:1234>
'''))


def test_main():
support.run_unittest(__name__)
# Remove global references to avoid looking like we have refleaks.
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Library/2018-05-23-00-26-27.bpo-11874.glK5iP.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Use a better regex when breaking usage into wrappable parts. Avoids bogus
assertion errors from custom metavar strings.