From 6ea70ef09a6723333d48aaabd59e1c47496f8755 Mon Sep 17 00:00:00 2001 From: Yang An Date: Wed, 29 Apr 2026 11:12:14 +1000 Subject: [PATCH 1/4] Fix help token expansion and percent handling --- knack/help.py | 23 ++++++++++++++++++++-- knack/parser.py | 46 ++++++++++++++++++++++++++++++++++++++++++++ tests/test_help.py | 46 ++++++++++++++++++++++++++++++++++++++++++++ tests/test_parser.py | 16 +++++++++++++++ 4 files changed, 129 insertions(+), 2 deletions(-) diff --git a/knack/help.py b/knack/help.py index ead4bfb..043a857 100644 --- a/knack/help.py +++ b/knack/help.py @@ -280,7 +280,7 @@ def __init__(self, help_ctx, delimiters, parser): 'deprecate_info': getattr(action, 'deprecate_info', None), 'preview_info': getattr(action, 'preview_info', None), 'experimental_info': getattr(action, 'experimental_info', None), - 'description': action.help, + 'description': self._expand_action_help(action), 'choices': action.choices, 'required': False, 'default': None, @@ -291,9 +291,28 @@ def __init__(self, help_ctx, delimiters, parser): help_param = next(p for p in self.parameters if p.name == '--help -h') help_param.group_name = 'Global Arguments' + @staticmethod + def _expand_action_help(action): + """Expand argparse-style help placeholders for Knack-rendered help.""" + if not isinstance(action.help, str) or '%' not in action.help: + return action.help + + parser = getattr(action.container, '_parser', None) + prog = getattr(parser, 'prog', '') + params = dict(vars(action), prog=prog) + for key in list(params): + if params[key] is argparse.SUPPRESS: + del params[key] + + try: + return action.help % params + except (KeyError, TypeError, ValueError): + # Keep help resilient even when token expansion cannot be resolved. + return action.help.replace('%%', '%') + def _add_parameter_help(self, param): param_kwargs = { - 'description': param.help, + 'description': self._expand_action_help(param), 'choices': param.choices, 'required': param.required, 'default': param.default, diff --git a/knack/parser.py b/knack/parser.py index 4883b00..7ec8aef 100644 --- a/knack/parser.py +++ b/knack/parser.py @@ -32,6 +32,50 @@ class CLICommandParser(argparse.ArgumentParser): + @staticmethod + def _sanitize_help_for_argparse(help_text): + """Escape literal '%' while preserving argparse mapping placeholders. + + argparse interpolates help text with ``%`` formatting against a dict. + Keep valid ``%(name)s``-style placeholders as-is and escape everything + else so help text like date formats (for example, ``%Y-%m-%d``) doesn't + crash during parser construction. + """ + if not isinstance(help_text, str) or '%' not in help_text: + return help_text + + result = [] + idx = 0 + text_len = len(help_text) + while idx < text_len: + char = help_text[idx] + if char != '%': + result.append(char) + idx += 1 + continue + + # Keep already-escaped percent signs as-is. + if idx + 1 < text_len and help_text[idx + 1] == '%': + result.append('%%') + idx += 2 + continue + + # Preserve mapping placeholders, e.g. %(default)s. + if idx + 1 < text_len and help_text[idx + 1] == '(': + closing_paren = help_text.find(')', idx + 2) + if closing_paren != -1 and closing_paren + 1 < text_len: + conversion_char = help_text[closing_paren + 1] + if conversion_char.isalpha(): + result.append(help_text[idx:closing_paren + 2]) + idx = closing_paren + 2 + continue + + # Any other '%' is literal and must be escaped for argparse. + result.append('%%') + idx += 1 + + return ''.join(result) + @staticmethod def create_global_parser(cli_ctx=None): global_parser = argparse.ArgumentParser(prog=cli_ctx.name, add_help=False) @@ -43,6 +87,8 @@ def create_global_parser(cli_ctx=None): def _add_argument(obj, arg): """ Only pass valid argparse kwargs to argparse.ArgumentParser.add_argument """ argparse_options = {name: value for name, value in arg.options.items() if name in ARGPARSE_SUPPORTED_KWARGS} + if 'help' in argparse_options: + argparse_options['help'] = CLICommandParser._sanitize_help_for_argparse(argparse_options['help']) if arg.options_list: scrubbed_options_list = [] for item in arg.options_list: diff --git a/tests/test_help.py b/tests/test_help.py index c115b2d..c5c2946 100644 --- a/tests/test_help.py +++ b/tests/test_help.py @@ -69,6 +69,9 @@ def load_command_table(self, args): g.command('n3', 'example_handler') g.command('n4', 'example_handler') g.command('n5', 'example_handler') + g.command('n6', 'example_handler') + g.command('n7', 'example_handler') + g.command('n8', 'example_handler') with CommandGroup(self, 'group alpha', '{}#{{}}'.format(__name__)) as g: g.command('n1', 'example_handler') @@ -90,6 +93,16 @@ def load_arguments(self, command): c.argument('arg2', options_list=['--foobar2'], required=True) c.argument('arg3', options_list=['--foobar3'], help='the foobar3') + with ArgumentsContext(self, 'n6') as c: + c.argument('arg1', options_list=['--fmt'], default='my-default', + help='default=%(default)s prog=%(prog)s') + + with ArgumentsContext(self, 'n7') as c: + c.argument('arg1', options_list=['--pct'], help='ratio 100%%') + + with ArgumentsContext(self, 'n8') as c: + c.argument('arg1', options_list=['--bad'], help='bad % token') + super().load_arguments(command) helps['n2'] = """ @@ -400,6 +413,39 @@ def test_help_group_help(self): expected = expected.format(self.cli_ctx.name) self.assertEqual(actual, expected) + @redirect_io + def test_help_argparse_default_and_prog_placeholders(self): + """Ensure argparse placeholders are expanded in help text.""" + + with self.assertRaises(SystemExit): + self.cli_ctx.invoke('n6 -h'.split()) + + actual = self.io.getvalue() + self.assertIn('Default=my-default prog=', actual) + self.assertNotIn('%(default)s', actual) + self.assertNotIn('%(prog)s', actual) + + @redirect_io + def test_help_argparse_escaped_percent(self): + """Ensure escaped percent signs render as a single literal percent.""" + + with self.assertRaises(SystemExit): + self.cli_ctx.invoke('n7 -h'.split()) + + actual = self.io.getvalue() + self.assertIn('Ratio 100%.', actual) + self.assertNotIn('Ratio 100%%.', actual) + + @redirect_io + def test_help_argparse_bad_percent_falls_back(self): + """Ensure malformed formatting falls back to the original help text.""" + + with self.assertRaises(SystemExit): + self.cli_ctx.invoke('n8 -h'.split()) + + actual = self.io.getvalue() + self.assertIn('Bad % token.', actual) + @redirect_io @mock.patch('knack.cli.CLI.register_event') def test_help_global_params(self, _): diff --git a/tests/test_parser.py b/tests/test_parser.py index a491b97..2a33452 100644 --- a/tests/test_parser.py +++ b/tests/test_parser.py @@ -180,6 +180,22 @@ def remove_test_file(file): remove_test_file('test.json') + def test_help_string_with_literal_percent_does_not_crash(self): + def test_handler(): + pass + + command = CLICommand(self.mock_ctx, 'test command', test_handler) + command.add_argument('date_fmt', '--date-fmt', help='Expected format: %Y-%m-%d') + cmd_table = {'test command': command} + self.mock_ctx.commands_loader.command_table = cmd_table + + parser = CLICommandParser() + parser.load_command_table(self.mock_ctx.commands_loader) + + def test_help_string_preserves_argparse_placeholders(self): + sanitized = CLICommandParser._sanitize_help_for_argparse('default is %(default)s (100% expected)') + self.assertEqual(sanitized, 'default is %(default)s (100%% expected)') + class VerifyError(object): # pylint: disable=too-few-public-methods From fb40a8bca99ef7975ca7f955f6c3782baba02195 Mon Sep 17 00:00:00 2001 From: Yang An Date: Tue, 12 May 2026 11:38:16 +1000 Subject: [PATCH 2/4] Simplify help % sanitization heuristic Adopt reviewer-recommended behavior: if help text already contains '%(' placeholders or '%%', keep it unchanged; otherwise escape '%' to '%%'. Update parser unit test expectation for mixed placeholder+literal percent string. Context: static scan across azure-cli-extensions help definitions found no mixed placeholder/literal percent cases in help lines, making this policy acceptable with existing CI help validation. --- knack/parser.py | 45 ++++++++------------------------------------ tests/test_parser.py | 2 +- 2 files changed, 9 insertions(+), 38 deletions(-) diff --git a/knack/parser.py b/knack/parser.py index 7ec8aef..032e053 100644 --- a/knack/parser.py +++ b/knack/parser.py @@ -34,47 +34,18 @@ class CLICommandParser(argparse.ArgumentParser): @staticmethod def _sanitize_help_for_argparse(help_text): - """Escape literal '%' while preserving argparse mapping placeholders. + """Escape literal '%' for argparse unless the author already escaped. - argparse interpolates help text with ``%`` formatting against a dict. - Keep valid ``%(name)s``-style placeholders as-is and escape everything - else so help text like date formats (for example, ``%Y-%m-%d``) doesn't - crash during parser construction. + If a help string already contains ``%(...)`` placeholders or ``%%``, + keep it as-is and rely on argparse's native formatting behavior. + Otherwise, escape ``%`` so literal percent tokens don't break help + processing in newer Python versions. """ if not isinstance(help_text, str) or '%' not in help_text: return help_text - - result = [] - idx = 0 - text_len = len(help_text) - while idx < text_len: - char = help_text[idx] - if char != '%': - result.append(char) - idx += 1 - continue - - # Keep already-escaped percent signs as-is. - if idx + 1 < text_len and help_text[idx + 1] == '%': - result.append('%%') - idx += 2 - continue - - # Preserve mapping placeholders, e.g. %(default)s. - if idx + 1 < text_len and help_text[idx + 1] == '(': - closing_paren = help_text.find(')', idx + 2) - if closing_paren != -1 and closing_paren + 1 < text_len: - conversion_char = help_text[closing_paren + 1] - if conversion_char.isalpha(): - result.append(help_text[idx:closing_paren + 2]) - idx = closing_paren + 2 - continue - - # Any other '%' is literal and must be escaped for argparse. - result.append('%%') - idx += 1 - - return ''.join(result) + if '%(' in help_text or '%%' in help_text: + return help_text + return help_text.replace('%', '%%') @staticmethod def create_global_parser(cli_ctx=None): diff --git a/tests/test_parser.py b/tests/test_parser.py index 2a33452..d903424 100644 --- a/tests/test_parser.py +++ b/tests/test_parser.py @@ -194,7 +194,7 @@ def test_handler(): def test_help_string_preserves_argparse_placeholders(self): sanitized = CLICommandParser._sanitize_help_for_argparse('default is %(default)s (100% expected)') - self.assertEqual(sanitized, 'default is %(default)s (100%% expected)') + self.assertEqual(sanitized, 'default is %(default)s (100% expected)') class VerifyError(object): # pylint: disable=too-few-public-methods From a939ced565251857879ad08ec573bf2544fd23fb Mon Sep 17 00:00:00 2001 From: YangAn-microsoft Date: Tue, 12 May 2026 17:34:31 +1000 Subject: [PATCH 3/4] Apply suggestion from @jiasli Co-authored-by: Jiashuo Li <4003950+jiasli@users.noreply.github.com> --- tests/test_parser.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_parser.py b/tests/test_parser.py index d903424..bf5da2b 100644 --- a/tests/test_parser.py +++ b/tests/test_parser.py @@ -193,8 +193,8 @@ def test_handler(): parser.load_command_table(self.mock_ctx.commands_loader) def test_help_string_preserves_argparse_placeholders(self): - sanitized = CLICommandParser._sanitize_help_for_argparse('default is %(default)s (100% expected)') - self.assertEqual(sanitized, 'default is %(default)s (100% expected)') + sanitized = CLICommandParser._sanitize_help_for_argparse('default is %(default)s (100%% expected)') + self.assertEqual(sanitized, 'default is %(default)s (100%% expected)') class VerifyError(object): # pylint: disable=too-few-public-methods From 1b11e11f9f0f96380b6c30d923b52f27c0f91d79 Mon Sep 17 00:00:00 2001 From: Yang An Date: Tue, 12 May 2026 17:47:05 +1000 Subject: [PATCH 4/4] docs: add PR #300 to 0.14.0 HISTORY notes --- HISTORY.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/HISTORY.rst b/HISTORY.rst index 27d7c93..0c92d21 100644 --- a/HISTORY.rst +++ b/HISTORY.rst @@ -7,6 +7,7 @@ Release History ++++++ * Declare support for Python 3.14 and drop support for Python 3.9 (#296) +* Fix help text rendering for Python 3.14 argparse strict validation (#300) 0.13.0 ++++++