Skip to content

{Misc.} Fix help % token handling for Python 3.14 argparse compatibility #300

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

Open
YangAn-microsoft wants to merge 1 commit into
base: dev
Choose a base branch
from
+129 −2
Open

{Misc.} Fix help % token handling for Python 3.14 argparse compatibility #300

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
23 changes: 21 additions & 2 deletions knack/help.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down
46 changes: 46 additions & 0 deletions knack/parser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,50 @@

class CLICommandParser(argparse.ArgumentParser):

@staticmethod
def _sanitize_help_for_argparse(help_text):
Copy link
Copy Markdown
Member

@jiasli jiasli May 11, 2026

Choose a reason for hiding this comment

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

This function is a little bit too complex. A regular expression can do the job. AI response:

Yes — the whole _sanitize_help_for_argparse loop can be replaced by a single re.sub with a negative lookahead. The idea: match any % that is not followed by another % (already-escaped) and not followed by a %(name)x mapping placeholder, then replace it with %%.

import re

# Matches a single '%' that is NOT:
#   - followed by another '%'  (already escaped: %%)
#   - the start of a mapping placeholder like %(default)s, %(prog)r, etc.
_LITERAL_PERCENT_RE = re.compile(r'%(?!%|\([^()]+\)[A-Za-z])')

@staticmethod
def _sanitize_help_for_argparse(help_text):
    if not isinstance(help_text, str) or '%' not in help_text:
        return help_text
    return _LITERAL_PERCENT_RE.sub('%%', help_text)

How the lookahead reads:

  • %% → the % is followed by %, lookahead fails the "not" condition → left alone (the second % is then consumed on its own iteration; since it's followed by whatever came after %%, it usually matches and… wait — see caveat below).
  • %(default)s → the % is followed by (default)s, matches \([^()]+\)[A-Za-z] → left alone.
  • %Y, %, % , 100% → no match in lookahead → replaced with %%.

Important caveat about %%

A naive single-pass re.sub walks left-to-right and doesn't re-scan its own replacement, but it does advance past the matched % only. With the lookahead (?!%), the first % in %% is skipped, then the regex engine moves to the second %. That second % is followed by whatever comes next (e.g. end of string, a space, Y, etc.), so it would get doubled into %%, turning %% into %%%. ❌

To handle this correctly with one regex, match the %% pair explicitly and keep it, e.g. using an alternation + a callback:

_PERCENT_RE = re.compile(r'%%|%\([^()]+\)[A-Za-z]|%')

def _sanitize_help_for_argparse(help_text):
    if not isinstance(help_text, str) or '%' not in help_text:
        return help_text
    # Keep %% and %(name)x as-is; escape any other lone %.
    return _PERCENT_RE.sub(lambda m: m.group(0) if len(m.group(0)) > 1 else '%%', help_text)

This is equivalent in behavior to the hand-written loop in the PR but ~6 lines instead of ~40, and arguably easier to audit. The alternation order matters: %% and %(...)x are tried before the bare % fallback, so they "win" and are returned unchanged; only the lone % branch gets escaped.

If you'd like, I can open a follow-up PR replacing the loop with this regex version (plus keeping the existing tests, which already cover %(default)s, 100%%, and %Y-%m-%d).

Test result:

import re

_PERCENT_RE = re.compile(r'%%|%\([^()]+\)[A-Za-z]|%')

def _sanitize_help_for_argparse(help_text):
    if not isinstance(help_text, str) or '%' not in help_text:
        return help_text
    # Keep %% and %(name)x as-is; escape any other lone %.
    return _PERCENT_RE.sub(lambda m: m.group(0) if len(m.group(0)) > 1 else '%%', help_text)

print(_sanitize_help_for_argparse("A string with single %, double %%, and placeholders %(prog)s. Test test."))

Output:

A string with single %%, double %%, and placeholders %(prog)s. Test test.

Copy link
Copy Markdown
Member

@jiasli jiasli May 11, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

However, even this level of complexity is not necessary, as pointed out by #299 (comment), the sanitization can be achieved as below:

  1. If there is %( or %% in the help string, leave the string as is, because this indicates the help string author is aware of the escaping rule.
  2. If not, escape % to %%, like AWS CLI did (Add support for Python 3.14 aws/aws-cli#9790).

It is not necessary to handle "a string with both % and %%". Just let it fail!

Copy link
Copy Markdown
Member

@jiasli jiasli May 11, 2026

Choose a reason for hiding this comment

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

AI comments on the above approach :

Yes, the comment is valid — and arguably the best approach for this PR. It trades a bit of edge-case coverage for a large reduction in complexity, with a clear and defensible rule.

The proposed rule

  1. If the help string contains %( or %%, the author already knows about argparse's escaping → leave it alone.
  2. Otherwise, blindly escape every % → %%.

Why it's valid

It's a "signal of intent" heuristic. The presence of %( or %% is a strong indicator that the author wrote argparse-aware help. If they didn't use either, they almost certainly meant % literally (%Y-%m-%d, 50% off, ratio 100%, etc.).

Implementation collapses to ~5 lines, no regex needed:

@staticmethod
def _sanitize_help_for_argparse(help_text):
    if not isinstance(help_text, str) or '%' not in help_text:
        return help_text
    if '%(' in help_text or '%%' in help_text:
        return help_text  # author handled escaping themselves
    return help_text.replace('%', '%%')

Covers all the realistic cases in the PR's tests:

Input Has %(/%%? Output argparse result
default=%(default)s prog=%(prog)s yes unchanged ✅ expands
ratio 100%% yes unchanged ✅ renders 100%
bad % token no bad %% token ✅ renders bad % token
Expected format: %Y-%m-%d no Expected format: %%Y-%%m-%%d ✅ renders literally

The conscious trade-off

Strings that mix both — e.g. "Use %Y-%m-%d, default %(default)s" — fall through rule 1 and crash argparse. The comment explicitly says "just let it fail!" That's reasonable because:

  • Such mixing is extremely rare in practice.
  • The failure is loud at parser-construction time, not silent corruption.
  • The author can fix it by escaping (%%Y-%%m-%%d).

How it compares to the regex approaches we discussed

Approach LOC Handles %Y + %(default)s mixed Cognitive load
Hand-written loop (current PR) ~40 yes medium
Alternation regex + lambda ~5 yes medium
Lookbehind/lookahead regex ~5 partial (%%% edge) high
%(/%% heuristic (this comment) ~5 no (by design) low

So the comment is valid both technically and as a design judgment. I'd recommend adopting it, and updating the n8 test ('bad % token') to assert the escaped-rendering path, plus dropping the n7/%% test's expectation since %% strings are now passed through untouched (which still renders as % via argparse — same observable result).

"""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)
Expand All @@ -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:
Expand Down
46 changes: 46 additions & 0 deletions tests/test_help.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand All @@ -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'] = """
Expand Down Expand Up @@ -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, _):
Expand Down
16 changes: 16 additions & 0 deletions tests/test_parser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down