Conversation
hugovk
left a comment
There was a problem hiding this comment.
Nice improvement!
It could be useful to have a general PIP_COLORS rather than PIP_NO_COLOR. Compare CPython's PYTHON_COLORS and pytest's PY_COLORS.
I find the --args cyan too close to the <name> dark cyan. In fact, I didn't even spot the difference at first:
Maybe use something more contrasting?
A comparison with argparse help, which uses green for short options and cyan for long:
Similar to Typer:
I agree, but this isn't the right PR to add that. Anyway, I implemented your suggestions, opting to reuse the colour scheme used by argparse.
The green short and cyan long options are still similar, but that's because I have my terminal ANSI colours set weirdly (I choose what looked pretty over contrast 🙂). |
|
The blue blends with the option colours, but sure. If matching CPython is what we want to do, we can do that. |
|
Is anyone else interested in reviewing this PR? If not, I'll probably merge this at a later date (once I double check behaviour on Windows). The idea was already approved in #12143 and now this PR cleans up the implementation. It is still a bit more complicated than I would've wished, but this part of the codebase is incredibly stable so it has minimal impact on maintenance. |
Annnnnd this is indeed broken on legacy Windows console. It should be fixable by replacing the weird console capturing logic with our pre-existing pip console infrastructure though. It should be a straightforward change, but will touch many lines of this PR so I'll get around to that later. |
Use Rich color markup within our help output. Unfortunately, I had to copy the format_option() method from optparse as it needs to be patched to ignore the Rich `[...]` markup tags while calculating length for text wrapping (otherwise the help is wrapped horribly). While this patch shares very little code with Ali's original patch, they do deserve credit for the idea and initial implementation. Co-authored-by: Ali Hamdan <ali.hamdan.dev@gmail.com>
pradyunsg
left a comment
There was a problem hiding this comment.
It would be good to have tests to validate the behaviours here!
|
Here's my test, added to the bottom of @pytest.mark.parametrize("envvar", ["", "NO_COLOR", "PIP_NO_COLOR"])
def test_help_command_colors(
in_memory_pip: InMemoryPip, monkeypatch: pytest.MonkeyPatch, envvar: str
) -> None:
"""
Test if color disables correctly.
"""
monkeypatch.delenv("NO_COLOR", raising=False)
monkeypatch.delenv("PIP_NO_COLOR", raising=False)
monkeypatch.delenv("FORCE_COLOR", raising=False)
PipConsole = pip._internal.cli.parser.PipConsole
TestConsole = functools.partial(
PipConsole, force_terminal=True, color_system="standard", width=80
)
monkeypatch.setattr(pip._internal.cli.parser, "PipConsole", TestConsole)
if envvar:
monkeypatch.setenv(envvar, "1")
res = in_memory_pip.pip("help")
text = Text.from_ansi(res.stdout)
bold_spans = [s for s in text.spans if getattr(s.style, "bold", None)]
bold_text = [text.plain[s.start : s.end] for s in bold_spans]
assert bold_text == ["Usage:", "Commands:", "General Options:"]
for span in bold_spans:
style = span.style
assert not isinstance(style, str)
if envvar:
assert style.color is None
else:
assert style.color is not NoneYou'll also need to adjust the imports: diff --git a/tests/functional/test_help.py b/tests/functional/test_help.py
index cba036927..802e0567e 100644
--- a/tests/functional/test_help.py
+++ b/tests/functional/test_help.py
@@ -1,7 +1,11 @@
+import functools
from unittest.mock import Mock
import pytest
+from pip._vendor.rich.text import Text
+
+import pip._internal.cli.parser
from pip._internal.cli.status_codes import ERROR, SUCCESS
from pip._internal.commands import commands_dict, create_command
from pip._internal.exceptions import CommandError
@@ -116,3 +120,41 @@ def test_help_commands_equally_functional(in_memory_pip: InMemoryPip) -> None:I also added |
|
And unit tests in import optparse
import pip._internal.cli.parser
def test_color_formatter_option_strings() -> None:
formatter = pip._internal.cli.parser.PrettyHelpFormatter()
strs = formatter.format_option_strings(
optparse.Option(
"--test-option",
"-t",
help="A test option for colors",
)
)
assert (
"[optparse.shortargs]-t[/], [optparse.longargs]--test-option[/]"
" [optparse.metavar]<test_option>[/]" == strs
)
def test_color_formatter_usage() -> None:
formatter = pip._internal.cli.parser.PrettyHelpFormatter()
usage = formatter.format_usage("usage: %prog [options] arg1 arg2")
assert (
"\n[optparse.groups]Usage:[/] usage: %prog \\[options] arg1 arg2\n" == usage
)
def test_color_formatter_section_heading() -> None:
formatter = pip._internal.cli.parser.PrettyHelpFormatter()
assert formatter.format_heading("Options") == ""
heading = formatter.format_heading("Other")
assert "[optparse.groups]Other:[/]\n" == heading
def test_color_formatter_description() -> None:
formatter = pip._internal.cli.parser.PrettyHelpFormatter()
description = formatter.format_description("This is a test description.")
assert (
"[optparse.groups]Description:[/]\n This is a test description.\n"
== description
)
def test_color_formatter_expand_defaults() -> None:
parser = pip._internal.cli.parser.CustomOptionParser(
formatter=pip._internal.cli.parser.PrettyHelpFormatter()
)
option = optparse.Option(
"--example",
help="An example option [default: %default]",
default="default_value",
)
parser.add_option(option)
expanded = parser.formatter.expand_default(option)
assert "An example option \\[default: default_value]" == expanded |
Co-authored-by: Henry Schreiner <henryschreineriii@gmail.com>
|
Thanks for the tests @henryiii! If you're okay with it, I'd like to get this in for pip 26.0 @notatallshaw. |
There was a problem hiding this comment.
Colors are cool but I'm far from an expert and don't really want to weigh in too much. The code looks fine so I am approving this.
I will say, I did try it all of VS Code's default themes I think it does make the options in light mode terminals a tiny bit less readable on my screen, where as in dark modes the options felt easier to read.
I'm not sure if a screenshot will help, this because I think it's my monitor's sub-pixel layout, VS Code's rendering choices, and my aging eye sight that come into play here, but there's one anyway.
Like I say, if there is a readability difference it is tiny.
|
It's approved, the code looks good, we have tests, multiple people like the look of it, merging. |



Supersedes PR #12143. I've copied the work done in the original PR, but simplified the code and added support for
PIP_NO_COLORandNO_COLORsince I imagine some people won't want the colours.