Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Take the FORCE_COLOR and NO_COLOR environment variables into account #9128

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions doc/user_guide/usage/output.rst
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,19 @@ a colorized report to stdout at the same time:

--output-format=json:somefile.json,colorized

Environment Variables
''''''''''''''''''''''''''''
The colorization of the output can also be controlled through environment
variables. The precedence for determining output format is as follows:

1. ``NO_COLOR``
2. ``FORCE_COLOR``, or ``PY_COLORS``
3. ``--output-format=...``

Setting ``NO_COLOR`` (to any value) will disable colorized output, while
``FORCE_COLOR`` or ``PY_COLORS`` (to any value) will enable it, overriding
the ``--output-format`` option if specified.


Custom message formats
''''''''''''''''''''''
Expand Down
5 changes: 5 additions & 0 deletions doc/whatsnew/fragments/3995.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Support for ``NO_COLOR`` and ``FORCE_COLOR`` (and ``PY_COLORS``, as an alias to ``FORCE_COLOR``) environment variables has been added.
When running `pylint`, the reporter that reports to ``stdout`` will be modified according to the requested mode.
The order is: ``NO_COLOR`` > ``FORCE_COLOR``, ``PY_COLORS`` > ``--output=...``.

Closes #3995 (https://github.com/pylint-dev/pylint/issues/3995).
Pierre-Sassoulas marked this conversation as resolved.
Show resolved Hide resolved
77 changes: 76 additions & 1 deletion pylint/lint/pylinter.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import sys
import tokenize
import traceback
import warnings
from collections import defaultdict
from collections.abc import Callable, Iterable, Iterator, Sequence
from io import TextIOWrapper
Expand Down Expand Up @@ -48,13 +49,15 @@
report_total_messages_stats,
)
from pylint.lint.utils import (
_is_env_set_and_non_empty,
augmented_sys_path,
get_fatal_error_message,
prepare_crash_report,
)
from pylint.message import Message, MessageDefinition, MessageDefinitionStore
from pylint.reporters import ReporterWarning
from pylint.reporters.base_reporter import BaseReporter
from pylint.reporters.text import TextReporter
from pylint.reporters.text import ColorizedTextReporter, TextReporter
from pylint.reporters.ureports import nodes as report_nodes
from pylint.typing import (
DirectoryNamespaceDict,
Expand All @@ -69,6 +72,14 @@

MANAGER = astroid.MANAGER

NO_COLOR = "NO_COLOR"
FORCE_COLOR = "FORCE_COLOR"
PY_COLORS = "PY_COLORS"

WARN_FORCE_COLOR_SET = "FORCE_COLOR is set; ignoring `text` at stdout"
WARN_NO_COLOR_SET = "NO_COLOR is set; ignoring `colorized` at stdout"
WARN_BOTH_COLOR_SET = "Both NO_COLOR and FORCE_COLOR are set! (disabling colors)"


class GetAstProtocol(Protocol):
def __call__(
Expand Down Expand Up @@ -250,6 +261,68 @@
}


def _handle_force_color_no_color(reporter: list[reporters.BaseReporter]) -> None:
"""
Check ``NO_COLOR``, ``FORCE_COLOR``, ``PY_COLOR`` and modify the reporter list
accordingly.

Rules are presented in this table:
+--------------+---------------+-----------------+------------------------------------------------------------+
| `NO_COLOR` | `FORCE_COLOR` | `output-format` | Behavior |
+==============+===============+=================+============================================================+
| `bool: True` | `bool: True` | colorized | not colorized + warnings (override + inconsistent env var) |

Check notice on line 273 in pylint/lint/pylinter.py

View workflow job for this annotation

GitHub Actions / pylint

C0402

Wrong spelling of a word 'env' in a docstring:
DanielNoord marked this conversation as resolved.
Show resolved Hide resolved
| `bool: True` | `bool: True` | / | not colorized + warnings (inconsistent env var) |

Check notice on line 274 in pylint/lint/pylinter.py

View workflow job for this annotation

GitHub Actions / pylint

C0402

Wrong spelling of a word 'env' in a docstring:
| unset | `bool: True` | colorized | colorized |
| unset | `bool: True` | / | colorized + warnings (override) |
| `bool: True` | unset | colorized | not colorized + warnings (override) |
| `bool: True` | unset | / | not colorized |
| unset | unset | colorized | colorized |
| unset | unset | / | not colorized |
+--------------+---------------+-----------------+------------------------------------------------------------+
"""
no_color = _is_env_set_and_non_empty(NO_COLOR)
force_color = _is_env_set_and_non_empty(FORCE_COLOR) or _is_env_set_and_non_empty(
PY_COLORS
)

if no_color and force_color:
warnings.warn(
WARN_BOTH_COLOR_SET,
ReporterWarning,
stacklevel=2,
)
force_color = False

if no_color:
for idx, rep in enumerate(list(reporter)):
if not isinstance(rep, ColorizedTextReporter):
continue

if rep.out.buffer is sys.stdout.buffer:
warnings.warn(
WARN_NO_COLOR_SET,
ReporterWarning,
stacklevel=2,
)
reporter.pop(idx)
reporter.append(TextReporter())

elif force_color:
for idx, rep in enumerate(list(reporter)):
# pylint: disable=unidiomatic-typecheck # Want explicit type check
if type(rep) is not TextReporter:
continue

if rep.out.buffer is sys.stdout.buffer:
warnings.warn(
WARN_FORCE_COLOR_SET,
ReporterWarning,
stacklevel=2,
)
reporter.pop(idx)
reporter.append(ColorizedTextReporter())


# pylint: disable=too-many-instance-attributes,too-many-public-methods
class PyLinter(
_ArgumentsManager,
Expand Down Expand Up @@ -435,6 +508,8 @@
# Extend the lifetime of all opened output files
close_output_files = stack.pop_all().close

_handle_force_color_no_color(sub_reporters)

if len(sub_reporters) > 1 or output_files:
self.set_reporter(
reporters.MultiReporter(
Expand Down
6 changes: 6 additions & 0 deletions pylint/lint/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from __future__ import annotations

import contextlib
import os
import platform
import sys
import traceback
Expand Down Expand Up @@ -133,3 +134,8 @@ def augmented_sys_path(additional_paths: Sequence[str]) -> Iterator[None]:
yield
finally:
sys.path[:] = original


def _is_env_set_and_non_empty(env_var: str) -> bool:
"""Checks if env_var is set and non-empty."""
return bool(os.environ.get(env_var))
4 changes: 4 additions & 0 deletions pylint/reporters/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@
from pylint.lint.pylinter import PyLinter


class ReporterWarning(Warning):
"""Warning class for reporters."""


def initialize(linter: PyLinter) -> None:
"""Initialize linter with reporters in this package."""
utils.register_plugins(linter, __path__[0])
Expand Down
151 changes: 150 additions & 1 deletion tests/lint/test_pylinter.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,36 @@
# For details: https://github.com/pylint-dev/pylint/blob/main/LICENSE
# Copyright (c) https://github.com/pylint-dev/pylint/blob/main/CONTRIBUTORS.txt

from __future__ import annotations

import io
import os
import sys
import warnings
from pathlib import Path
from typing import Any, NoReturn
from unittest import mock
from unittest.mock import patch

import pytest
from pytest import CaptureFixture

from pylint.lint.pylinter import MANAGER, PyLinter
from pylint.lint.pylinter import (
FORCE_COLOR,
MANAGER,
NO_COLOR,
PY_COLORS,
WARN_BOTH_COLOR_SET,
PyLinter,
_handle_force_color_no_color,
)
from pylint.reporters.text import ColorizedTextReporter, TextReporter
from pylint.utils import FileState

COLORIZED_REPORTERS = "colorized_reporters"
TEXT_REPORTERS = "text_reporters"
STDOUT_TEXT = "stdout"


def raise_exception(*args: Any, **kwargs: Any) -> NoReturn:
raise ValueError
Expand Down Expand Up @@ -68,3 +87,133 @@ def test_open_pylinter_prefer_stubs(linter: PyLinter) -> None:
assert MANAGER.prefer_stubs
finally:
MANAGER.prefer_stubs = False


def pytest_generate_tests(metafunc: pytest.Metafunc) -> None:
if metafunc.function.__name__ != test_handle_force_color_no_color.__name__:
return

if (
TEXT_REPORTERS not in metafunc.fixturenames
or COLORIZED_REPORTERS not in metafunc.fixturenames
):
warnings.warn(
f"Missing fixture {TEXT_REPORTERS} or {COLORIZED_REPORTERS} in"
f" {test_handle_force_color_no_color.function.__name__}??",
stacklevel=2,
)
return

parameters = []

reporter_combinations = [
("file", STDOUT_TEXT),
("file",),
(STDOUT_TEXT,),
("",),
]

for tr in list(reporter_combinations):
for cr in list(reporter_combinations):
tr = tuple(t for t in tr if t)
cr = tuple(t for t in cr if t)

total_reporters = len(tr) + len(cr)
unique_reporters = len(set(tr + cr))

if total_reporters == 0:
continue

if unique_reporters != total_reporters:
continue

parameters.append((tuple(tr), tuple(cr)))

metafunc.parametrize(
f"{TEXT_REPORTERS}, {COLORIZED_REPORTERS}", parameters, ids=repr
)


@pytest.mark.parametrize(
"no_color",
[True, False],
ids=lambda no_color: f"{no_color=}",
)
@pytest.mark.parametrize(
"py_colors",
[True, False],
ids=lambda py_colors: f"{py_colors=}",
)
@pytest.mark.parametrize(
"force_color",
[True, False],
ids=lambda force_color: f"{force_color=}",
)
def test_handle_force_color_no_color(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd like to have @Pierre-Sassoulas's opinion on this test. This isn't really the pattern we would normally use (e.g., use of pytest_generate_tests) but I'm not sure how he would prefer this to be tested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was a bit too quick to "ask for help", since Windows tests fail (https://github.com/pylint-dev/pylint/actions/runs/12734784829/job/35492756662?pr=9128)

I was aiming very specifically at #9128 (comment) (which you kindly fix-up-ed yourself ❤️)

At least nowadays I have "somewhat" of Python setup in Windows and I could try to decipher this error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to have @Pierre-Sassoulas's opinion on this test. This isn't really the pattern we would normally use (e.g., use of pytest_generate_tests) but I'm not sure how he would prefer this to be tested.

But for the comment at-hand:

@pytest.mark.parametrize(
    "no_color",
    [True, False],
    ids=lambda no_color: f"{no_color=}",
)

is trivial to test like so. For attaching reporters, not so much.

I could ... try to run this code once, and @pytest.mark.parametrize with the output of the code (ie parameters) instead

Copy link
Member

Choose a reason for hiding this comment

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

I feel like the test is as (or more) complicated than the code we want to test, which is bad. A middle ground would be to parametrize only one parameter and hardcode the other one ? Something like:

@pytest.mark.parametrize(
    "force_color,expected",
    [(True, False)],
    ids=lambda force_color: f"{force_color=}",
)
def test_handle_force_color_no_color_true_text_reporter(...)
     monkeypatch.setenv(NO_COLOR, "1")
    reporter = TextReporter

@pytest.mark.parametrize(
    "force_color",
    [True, False],
    ids=lambda force_color: f"{force_color=}",
)
def test_handle_force_color_no_color_colorized_text_reporter(...)
     monkeypatch.setenv(NO_COLOR, "")
    reporter = ColorizedTextReporter

@pytest.mark.parametrize(
    "force_color,expected",
    [(True, False)],
    ids=lambda force_color: f"{force_color=}",
)
def test_handle_force_color_no_color_true_text_reporter(...)
     monkeypatch.setenv(NO_COLOR, "1")
     reporter = TextReporter

@pytest.mark.parametrize(
    "force_color",
    [True, False],
    ids=lambda force_color: f"{force_color=}",
)
def test_handle_force_color_no_color_colorized_text_reporter(...)
     monkeypatch.setenv(NO_COLOR, "")
    reporter = ColorizedTextReporter

I'm not sure if it's better than what Daniel suggest. Especially if we choose the parameter badly (we should parametrize the one that doesn't change anything because the other take precedance). Also it seems we don't actually test the color output but a warning instead?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I agree with Pierre that it would probably be better to bit more explicit and write out more of the code itself.

monkeypatch: pytest.MonkeyPatch,
recwarn: pytest.WarningsRecorder,
no_color: bool,
py_colors: bool,
force_color: bool,
text_reporters: tuple[str],
colorized_reporters: tuple[str],
) -> None:
monkeypatch.setenv(NO_COLOR, "1" if no_color else "")
monkeypatch.setenv(FORCE_COLOR, "1" if force_color else "")
monkeypatch.setenv(PY_COLORS, "1" if py_colors else "")

force_color = force_color or py_colors

if STDOUT_TEXT in text_reporters or STDOUT_TEXT in colorized_reporters:
monkeypatch.setattr(sys, STDOUT_TEXT, io.TextIOWrapper(io.BytesIO()))

reporters = []
for reporter, group in (
(TextReporter, text_reporters),
(ColorizedTextReporter, colorized_reporters),
):
for name in group:
if name == STDOUT_TEXT:
reporters.append(reporter())
if name == "file":
reporters.append(reporter(io.TextIOWrapper(io.BytesIO())))

_handle_force_color_no_color(reporters)

if no_color and force_color:
# Both NO_COLOR and FORCE_COLOR are set; expecting a warning.
both_color_warning = [
idx
for idx, w in enumerate(recwarn.list)
if WARN_BOTH_COLOR_SET in str(w.message)
]
assert len(both_color_warning) == 1
recwarn.list.pop(both_color_warning[0])

if no_color:
# No ColorizedTextReporter expected to be connected to stdout.
assert all(
not isinstance(rep, ColorizedTextReporter)
for rep in reporters
if rep.out.buffer is sys.stdout.buffer
)

if STDOUT_TEXT in colorized_reporters:
assert len(recwarn.list) == 1 # expect a warning for overriding stdout
else:
assert len(recwarn.list) == 0 # no warning expected
elif force_color:
# No TextReporter expected to be connected to stdout.
# pylint: disable=unidiomatic-typecheck # Want explicit type check.
assert all(
type(rep) is not TextReporter
for rep in reporters
if rep.out.buffer is sys.stdout.buffer
)

if STDOUT_TEXT in text_reporters:
assert len(recwarn.list) == 1 # expect a warning for overriding stdout
else:
assert len(recwarn.list) == 0 # no warning expected
else:
assert len(recwarn.list) == 0 # no warning expected
35 changes: 34 additions & 1 deletion tests/lint/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,16 @@

import unittest.mock
from pathlib import Path, PosixPath
from typing import Any

import pytest

from pylint.constants import full_version
from pylint.lint.utils import get_fatal_error_message, prepare_crash_report
from pylint.lint.utils import (
_is_env_set_and_non_empty,
get_fatal_error_message,
prepare_crash_report,
)
from pylint.testutils._run import _Run as Run


Expand Down Expand Up @@ -56,3 +61,31 @@ def test_issue_template_on_fatal_errors(capsys: pytest.CaptureFixture) -> None:
assert "Fatal error while checking" in captured.out
assert "Please open an issue" in captured.out
assert "Traceback" in captured.err


@pytest.mark.parametrize(
"value, expected",
[
(None, False),
("", False),
(0, True),
(1, True),
(2, True),
(False, True),
("no", True),
("off", True),
("on", True),
(True, True),
("yes", True),
],
ids=repr,
)
def test_is_env_set_and_non_empty(
monkeypatch: pytest.MonkeyPatch, value: Any, expected: bool
) -> None:
"""Test the function returns True if the environment variable is set and non-empty."""
env_var = "TEST_VAR"
if value is not None:
monkeypatch.setenv(env_var, str(value))

assert _is_env_set_and_non_empty(env_var) == expected
Loading