Take the ``FORCE_COLOR`` and ``NO_COLOR`` environnement variables into account #3995

Open devNan0 opened 3 years ago

Steps to reproduce

pylint --output-format=colorized project/

Current behavior

Output without color

Expected behavior

Output with color

pylint --version output

pylint --version
pylint 2.6.0
astroid 2.4.2
Python 3.9.1 (default, Dec 11 2020, 14:22:09) 
[GCC 8.3.0]

Additional information

I´m running pylint within an GitLab Pipeline

I noticed that if

stdout is not a tty, and
colorama package is installed

then output is not colored.

Steps to reproduce:

Create a sample Python source code file:

echo 'print(x)' > test1.py
Make sure colorama package is not installed.
Run pylint:

pylint --output-format=colorized test1.py > report1.log
Observe that report1.log contains colored output:

cat report1.log
Alternatively, find reset ANSI escape code:

$ grep -c 0m report1.log 3
Install package colorama
Run pylint:

pylint --output-format=colorized test1.py > report2.log
Observe that report2.log contains output without color:

cat report2.log
The report does not contain reset ANSI escape code:

$ grep -c 0m report2.log 0

Unfortunately, importing isort has a side effect of overwriting sys.stdout!

I added import sys as well as a couple print statements around import isort.api here https://github.com/PyCQA/pylint/blob/fce898e283107ff5a4f3dbf11c8927bef1a7333a/pylint/utils/utils.py#L6 :

    import sys
    print(f"before 'import isort.api': {sys.stdout = }")
    import isort.api
    print(f"after 'import isort.api': {sys.stdout = }")

Then I ran pylint:

pylint --output-format=colorized test1.py > report3.log

report3.log shows that sys.stdout is overwritten:

before 'import isort.api': sys.stdout = <_io.TextIOWrapper name='<stdout>' mode='w' encoding='utf-8'>
after 'import isort.api': sys.stdout = <colorama.ansitowin32.StreamWrapper object at 0x7fa853e46a30>

colorama source code indicates that it strips ANSI sequences in our usecase (see ansitowin32.py):

        # should we strip ANSI sequences from our output?
        if strip is None:
            strip = conversion_supported or (not self.stream.closed and not self.stream.isatty())
        self.strip = strip

Interestingly, if PYCHARM_HOSTED environment variable is set, colorama doesn't modify sys.stdout.

Not sure where is the right place to fix the problem.

It is a shame that pylint never managed to act on FORCE_COLOR=1, even if the number of tools supporting it is huge. A search on google on FORCE_COLOR returns >1.6M pages.

What is FORCE_COLOR @ssbarnea, I've never heard of it ? It seems it's a javascript or even nodejs convention ? We have a an option for colorized output that should work, I think this is what this issue is about.

FORCE_COLOR and NO_COLOR are a tandem of environment variables that application agnostic that are used to control coloring output.

Command line argument for controlling color were always prove to be hard or impossible to use because the user does not always have the power to change the final arguments of each called application (think about multiple layers). Instead environment variables are passed by default to subprocesses, so they care easier to use.

For example on CI you can easily define one of them in order to either activate or deactivate coloring, based on your supported features.

There is even a famous website https://no-color.org/ which popularized one of the options, but when they started they failed to think about the opposite use case, still searching only you will find that most tools added support for it.

Among those that I know to support it: rich, hatch, pytest, molecule, ansible-lint.

Apparently pylint does not support even the PY_COLORS which was very popular to but people argues that too limitative to python.

I guess we could say that NO_COLOR and FORCECOLOR are the least biased and most future approaches, at least they do not add the APP suffix which for these two was mistake because it would have never scale. If you have 200 cli tools, do you want to add 200 variables with APP_FORCE_COLOR? ;) - So far I am not aware of any practical use-case where it was need to enable colors for one app but not for others (even if that would be possible by overriding the vars).

I hope it helps. Feel free to look at one implementation from https://github.com/pycontribs/enrich/blob/main/src/enrich/console.py#L65 which implemented most known ones. That was written at a moment when rich itself did not has support, but later added making is less needed.

To give a practical example: when using pylint as a pre-commit hook, one must add the options inside pre-commit hook because pylint does not have a variable to control coloring. Basically the repo owner have to forced his preference inside the code, as there is no way to enabled/disable it without changing the code.

Implementing NO_COLORS alone would be nice already. It seems only a matter of patching https://github.com/PyCQA/pylint/blob/main/pylint/reporters/text.py#L114 to disable colors globally.

Workaround

Set an environment variable TERM=xterm-256color which will make pylint colored.

👉 TERM=xterm-256color pylint .

FORCE_COLOR and NO_COLOR are a tandem of environment variables that application agnostic that are used to control coloring output.

I'd PR those, if repo thinks they'd be interested.

Of course, it might get trickier with multiple output-formats (https://pylint.readthedocs.io/en/stable/user_guide/usage/output.html#output-options) - but it might be more easy actually to mess with those, rather than the suggested https://github.com/PyCQA/pylint/blob/main/pylint/reporters/text.py?rgh-link-date=2022-12-19T13%3A16%3A50Z#L114

I think we want to support FORCE_COLOR and NO_COLOR if there's a demand for it. But we also have an option to colorize the output already. So I think some design / specification before jumping into implementation is necessary.

What should happen for each possible combination of FORCE_COLOR / NO_COLOR / --output-format=colorized ? How can we make this consistent ? Are we making breaking changes to keep this sane (it's the proper season) ? Modifying the value of --output-format on the fly ? Raising warning when those options disagree with each others ?

FORCE_COLOR	NO_COLOR	output-format	Behavior
True	True	colorized	?
True	True	/	?
True	False	colorized	colorized
True	False	/	?
False	True	colorized	?
False	True	/	not colorized
False	False	colorized	?
False	False	/	?

What should happen for each possible combination of FORCE_COLOR / NO_COLOR/ --output-format=colorized ?

IMHO order is:

NO_COLOR: If we should be dumb, we must be dumb
FORCE_COLOR: Facilitate people trying to help themselves (PY_COLORS too?)
--output-format=colorized

How can we make this consistent ?

❔ 😅

Are we making breaking changes to keep this sane (it's the proper season) ?

Supporting at least NO_COLOR would be nice (there's a momentum in https://no-color.org/), I don't foresee that hurting much.

Indeed pylint is a nice player - its output is by-default ASCII, and can be colorized. My case with it goes:

I'd like that pylint by-default colorizes the terminal when terminal is a tty
There's no such thing, so we store --output-format=colorized in the repo .pylintrc
... but there is this user that wants it NO_COLOR
... or you'd like the CI NO_COLOR

Modifying the value of --output-format on the fly ?

That would be my go-to approach to solve this. I think hooking anywhere else might be too hard - since text/colorized are two different options. And --output-format is nowadays a csv-value 😕

Raising warning when those options disagree with each others ?

That we can do. Also notify the user that environment variables overrid the given configuration, if you think so

Hmm I think we have three states for each env var, false, true and not set. It gets pretty complicated, what do you think of this recap, did I understand what you said ?

FORCE_COLOR	NO_COLOR	output-format	Behavior
True	True	colorized	not colorized + warnings (override + inconsistent env var)
True	True	/	not colorized + warnings (inconsistent env var)
True	False	colorized	colorized
True	False	/	colorized + warnings (override)
True	unset	colorized	colorized
True	unset	/	colorized + warnings (override)
False	True	colorized	not colorized + warnings (override)
False	True	/	not colorized
False	False	colorized	colorized ?
False	False	/	not colorized ?
False	unset	colorized	colorized ?
False	unset	/	not colorized ?
unset	True	colorized	not colorized + warnings (override)
unset	True	/	not colorized
unset	False	colorized	colorized ?
unset	False	/	not colorized ?
unset	unset	colorized	colorized
unset	unset	/	not colorized

There's a good comment there summarizing how to use these env vars: https://github.com/termcolor/termcolor/pull/25#issuecomment-1709882191

I think it will be better to just go with "NO_COLOR" in os.environ and os.environ["NO_COLOR"] (or, I'd simplify it as os.environ.get("NO_COLOR") is not None). Same with FORCE_COLOR.

So, we end up with

`NO_COLOR`	`FORCE_COLOR`	`output-format`	Behavior
`is not None`	`is not None`	colorized	not colorized + warnings (override + inconsistent env var)
`is not None`	`is not None`	/	not colorized + warnings (inconsistent env var)
unset	`is not None`	colorized	colorized
unset	`is not None`	/	colorized + warnings (override)
`is not None`	unset	colorized	not colorized + warnings (override)
`is not None`	unset	/	not colorized
unset	unset	colorized	colorized
unset	unset	/	not colorized

which is more manageable. And also seems to be the consensus.

Including the warnings (if we go with them), there won't be any magic ✨ why X is happening.

PS: Thank you @hydrargyrum for the link 🙃

Maybe bool(envvar) instead if is not None ? So everything falsey works as probably intended ?

Indeed, https://no-color.org/ (and the aforementioned comment) states:

[…] NO_COLOR environment variable that, when present and not an empty string (regardless of its value), prevents the addition of ANSI color

Hmm so it's NO_COLOR not in [", None] then.

I didn't test it with code yet - but I promise that thin wrapper will have explicit tests 😅

pylint-dev / pylint