Skip to content

Conversation

@alexprengere
Copy link
Contributor

@alexprengere alexprengere commented Nov 25, 2025

@mpkocher
Copy link
Contributor

Consider adding a test or extending an existing test.

@alexprengere
Copy link
Contributor Author

Of course 😉 (this is a draft PR).
I will extend the color test in test_argparse, I wanted to be sure whether to color all interpolated values or just default.
Also I am unsure whether to re-use theme.default_value or introduce new values in _colorize.py like #141680 did.
I will clean this up tomorrow and then mark it as ready for review.

@alexprengere
Copy link
Contributor Author

I updated the code and tests 😉

I had to change a bit the logic introduced in #141680 as introducing a new color for interpolated values, that would be different from the default_value color, is a bit complex to handle.

For ArgumentDefaultsHelpFormatter, as _get_help_string returns the %(default)s already "colored", updating _expand_help to "color" interpolated values would essentially color the default values twice, and no longer respect the default_value color.
So I removed the coloring of %(default)s in _get_help_string, and now it happens in _expand_help, as the default value will be colored, along all interpolated values, using a sinple interpolated_value color.

I hope this is clear, I can amend this if needed.

@alexprengere
Copy link
Contributor Author

alexprengere commented Nov 28, 2025

For the record the PR could be slightly improved with the formatting of choices, by coloring each individual values before joining them with ', '.
I tried it but encountered bug in an unrelated part (filed in #142035):

image

The issue is that the use of textwrap.wrap happens after the ANSI codes have been inserted, causing the wrapping to be broken in some cases.

Copy link
Member

@savannahostrowski savannahostrowski left a comment

Choose a reason for hiding this comment

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

A couple of comments but I spent some time playing around with this and I think it looks pretty good. I think that we're early enough in the release cycle that we can see what, if any, feedback we get on the use of YELLOW for default, type and interpolated values. Some folks may find it overly busy but personally, I think having some colour treatment here does help with the overall readability.

FWIW, I also compared this to rich-argparse and click. Neither seem to do this type of interpolation in help text. Not necessarily a point for or against doing this but just wanted to note it!

@bedevere-app
Copy link

bedevere-app bot commented Dec 2, 2025

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@alexprengere
Copy link
Contributor Author

Thanks a lot for the review @savannahostrowski!
I have made the requested changes; please review again.

For the record, I was willing to also change:

# current
params[choices] = ', '.join(map(str, params['choices']))
# possible
def _color(s):
    return f"{t.interpolated_value}{s}{t.reset}"
params['choices'] = ', '.join(map(_color, params['choices']))

But this introduced more ANSI escape codes, and I kept hitting the wrapping bug as described in #142035:

image

So I left that part alone for now, but I can revisit if we ever find a solution.

@bedevere-app
Copy link

bedevere-app bot commented Dec 2, 2025

Thanks for making the requested changes!

@savannahostrowski: please review the changes made to this pull request.

Copy link
Member

@savannahostrowski savannahostrowski left a comment

Choose a reason for hiding this comment

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

Thanks for this! I'll let Hugo also take a look before merging but this LGTM!

@mpkocher
Copy link
Contributor

mpkocher commented Dec 2, 2025

@alexprengere A lot of attention to detail on this issue and it shows. Nice job.

# current
params[choices] = ', '.join(map(str, params['choices']))
# possible
def _color(s):
    return f"{t.interpolated_value}{s}{t.reset}"
params['choices'] = ', '.join(map(_color, params['choices']))

As a separate issue, when colorize is made public, I believe it would be useful to improve the ergonomics and add a sugar layer so that AsciiColors.Green("car") (or something similar) would translate to {AsciiColors.Green.value}car{AsciiColors.reset}. This current model of {something}value{t.reset} is a bit chatty with always calling .reset. We probably want to avoid people defining their own _color util func in their project/lib.

@savannahostrowski savannahostrowski linked an issue Dec 6, 2025 that may be closed by this pull request
Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

Thanks! Please could you fix the merge conflict?

@alexprengere
Copy link
Contributor Author

It shoud be fine now 👍

@savannahostrowski savannahostrowski enabled auto-merge (squash) December 12, 2025 16:33
@savannahostrowski
Copy link
Member

Thanks again for working on this @alexprengere! Appreciate it!

@savannahostrowski savannahostrowski merged commit 6d644e4 into python:main Dec 12, 2025
54 checks passed
t = self._theme
for name, value in params.items():
params[name] = f"{t.interpolated_value}{value}{t.reset}"
return help_string % params

Choose a reason for hiding this comment

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

The above change looks to be giving me issues at this line.

An MVE looks like this:

my_parser.add_argument(
        "--foo",
        type=int,
        default=1234,
        help=f"""0x%(default)x""",
    )

The above works in 3.14.2, but not in 3.15 alpha3. I apologize if my code is doing something incorrect.

The error I get is

Traceback (most recent call last):
  File "[...]/3.15.0a3/lib/python3.15/argparse.py", line 1793, in _check_help
    formatter._expand_help(action)
    ~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^
  File "[...]/3.15.0a3/lib/python3.15/argparse.py", line 696, in _expand_help
    return help_string % params
           ~~~~~~~~~~~~^~~~~~~~
TypeError: %x format: an integer is required, not str

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO your code is correct. Here is the basic issue:

# before 3.15a3
>>> """0x%(default)x""" % {'default': 1234}
'0x4d2'

# after 3.15a3
>>> """0x%(default)x""" % {'default': f"\x1b[1;31m1234"}
Traceback (most recent call last):
  File "<python-input-14>", line 1, in <module>
    """0x%(default)x""" % {'default': f"\x1b[1;31m1234"}
    ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
TypeError: %x format: an integer is required, not str

The value to interpolate is converted to a (colored) string first, then passed to the % formatting, where it fails due to a type mismatch (with x it expects an int).

If we want to keep this feature, I think the coloring needs to happen in help_string, so we do not fail on such cases, as we preserve the types of interpolated values. This is a bit more complex, but was already done for ArgumentDefaultsHelpFormatter in #141680 for _get_help_string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the record I created #142980 to track this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add colours to argparse interpolated values in --help

5 participants