allow CLICOLOR_FORCE env var to force CLI color output #6270
Conversation
cconverse711
force-pushed
the
clicolor-force
branch
from
d6bb2b2
to
0f4ba54
Compare
|
Thanks for your contribution. Please add a unit test for this. |
|
Shouldn't this implement |
|
hmm I am not sure if a unit test would be possible. not if getenv is cached. maybe you want to uncache it it testing somehow. or another alternative is to write a test in cppcheck/test/cli/test-other.py You could add a line in the releasenotes.txt also. And before we merge it it would not hurt if I add a ticket in trac. |
That's actually what I meant. It was still early. 😴 |
According to the full linked "proposal", yes, however most of the other tools that support |
cconverse711
force-pushed
the
clicolor-force
branch
from
9ad9c4c
to
c63cb27
Compare
I would prefer if you would do both while you are at it. |
|
I think your code and testing looks good. Adding the NO_COLOR sounds easy technically, I am in favor of adding it also :-). But I am not sure how you would add the test for it - ideally the test would provide a tty terminal right? |
|
Thanks. I think I figured out a decent way to test using tty, let me know if you'd prefer to omit that. |
|
I still want to take a look this weekend. Something seems off to me but I cannot tell what. |
| return os; | ||
| } | ||
|
|
||
| std::string toString(Color c) | ||
| { | ||
| #ifndef _WIN32 | ||
| std::stringstream ss; | ||
| std::ostringstream ss; |
There was a problem hiding this comment.
I was about to comment that I would prefer if we could do this without a stream but it turns out this function is only called to generate a static lookup table so it is actually fine.
Already commented on the "off" thing. Still need to take a look at the Python stuff. |
| @@ -71,6 +71,43 @@ def test_preprocessor_error(tmpdir): | |||
| assert exitcode != 0 | |||
|
|
|||
|
|
|||
| ANSI_BOLD = "\x1b[1m" | |||
There was a problem hiding this comment.
You could prefix them with __ so they are private.
|
|
||
| assert exitcode == 0 | ||
| assert stderr | ||
| assert (ANSI_BOLD in stderr) == color_expected |
There was a problem hiding this comment.
I usually prefer to check the exact output instead of just checking if something is contained. Makes it easier to understand what is actually returned. But as we are just verifying that the environment variables work it is fine.
| @@ -72,8 +75,82 @@ def __lookup_cppcheck_exe(): | |||
| return exe_path | |||
|
|
|||
|
|
|||
| def __run_subprocess_tty(args, env=None, cwd=None, timeout=None): | |||
There was a problem hiding this comment.
Still need to review this. Everything else LGTM.
There was a problem hiding this comment.
I have not verified all the behavior of the calls in detail but it looks fine. Also it is only used by the added tests and the CI works so even if there is a small oversight it would be very limited.
| raise | ||
| # EIO means EOF on some systems | ||
| readable.remove(fd) | ||
| else: |
There was a problem hiding this comment.
try: -> except: -> else:? That seems extremely errorprone as it looks like an incorrectly intended block.
No need to change it.
| @@ -72,8 +75,82 @@ def __lookup_cppcheck_exe(): | |||
| return exe_path | |||
|
|
|||
|
|
|||
| def __run_subprocess_tty(args, env=None, cwd=None, timeout=None): | |||
There was a problem hiding this comment.
I have not verified all the behavior of the calls in detail but it looks fine. Also it is only used by the added tests and the CI works so even if there is a small oversight it would be very limited.
|
Sorry the review took so long. |
This adds support for using the
CLICOLOR_FORCEenvironment variable to force color output in Linux even when a tty is not used, mimicking behavior in CMake and other tools. The primary intent is to allow color to be output when cppcheck is run in CI, for example.