| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
Consider adding a test or extending an existing test. |
Sorry, something went wrong.
|
Of course 😉 (this is a draft PR). |
Sorry, something went wrong.
|
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. I hope this is clear, I can amend this if needed. |
Sorry, something went wrong.
|
For the record the PR could be slightly improved with the formatting of choices, by coloring each individual values before joining them with ', '.
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. |
Sorry, something went wrong.
There was a problem hiding this comment.
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!
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
Thanks a lot for the review @savannahostrowski! 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:
So I left that part alone for now, but I can revisit if we ever find a solution. |
Sorry, something went wrong.
|
Thanks for making the requested changes! @savannahostrowski: please review the changes made to this pull request. |
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks for this! I'll let Hugo also take a look before merging but this LGTM!
Sorry, something went wrong.
|
@alexprengere A lot of attention to detail on this issue and it shows. Nice job.
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. |
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks! Please could you fix the merge conflict?
Sorry, something went wrong.
|
It shoud be fine now 👍 |
Sorry, something went wrong.
|
Thanks again for working on this @alexprengere! Appreciate it! |
Sorry, something went wrong.
| t = self._theme | ||
| for name, value in params.items(): | ||
| params[name] = f"{t.interpolated_value}{value}{t.reset}" | ||
| return help_string % params |
There was a problem hiding this comment.
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
Sorry, something went wrong.
There was a problem hiding this comment.
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 strThe 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.
Sorry, something went wrong.
There was a problem hiding this comment.
For the record I created #142980 to track this.
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Uh oh!
There was an error while loading. Please reload this page.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.