-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Take the FORCE_COLOR
and NO_COLOR
environment variables into account
#9128
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
base: main
Are you sure you want to change the base?
Changes from 10 commits
c6ca5e5
aec5efb
b5b91c1
7e12334
7f38e76
4427cf3
51cf8d2
6d558c7
8d8f988
bb87473
68b07a4
cf95d27
c09eb18
cd55074
a2afa05
260b671
abd545e
eadd83a
edb9812
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -108,6 +108,7 @@ encodings | |
endswith | ||
enum | ||
enums | ||
env | ||
epilog | ||
epylint | ||
epytext | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
Support for ``NO_COLOR`` and ``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`` > ``--output=...``. | ||
|
||
Closes #3995 (https://github.com/pylint-dev/pylint/issues/3995). | ||
Pierre-Sassoulas marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,17 +2,35 @@ | |
# 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, | ||
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 | ||
|
@@ -68,3 +86,124 @@ 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( | ||
"force_color", | ||
[True, False], | ||
ids=lambda force_color: f"{force_color=}", | ||
) | ||
def test_handle_force_color_no_color( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
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 "") | ||
|
||
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 |
Uh oh!
There was an error while loading. Please reload this page.