Take the `FORCE_COLOR` and `NO_COLOR` environment variables into account #9128

stdedos · 2023-10-07T20:44:18Z

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=....

Signed-off-by: Stavros Ntentos [email protected]

Document your change, if it is a non-trivial one.
- A maintainer might label the issue skip-news if the change does not need to be in the changelog.
- Otherwise, create a news fragment with towncrier create <IssueNumber>.<type> which will be
  included in the changelog. <type> can be one of the types defined in ./towncrier.toml.
  If necessary you can write details or offer examples on how the new change is supposed to work.
- Generating the doc is done with tox -e docs
Relate your change to an issue in the tracker if such an issue exists (Refs incorrect analysing of etree #1234, Closes incorrect analysing of etree #1234)
Write comprehensive commit messages and/or a good description of what the PR does.
Keep the change small, separate the consensual changes from the opinionated one.
Don't hesitate to open multiple PRs if the change requires it. If your review is so
big it requires to actually plan and allocate time to review, it's more likely
that it's going to go stale.
If you used multiple emails or multiple names when contributing, add your mails
and preferred name in script/.contributors_aliases.json

Type of Changes

	Type
✓	✨ New feature

Description

Fixes #3995

stdedos · 2023-10-09T06:13:07Z

Any ideas of how to detect that a reporter is attached to stdout that works on Windows too? 😓

hugovk · 2024-06-07T05:27:25Z

+1 to FORCE_COLOR and NO_COLOR.

I suggest to leave PY_COLORS out because it's pytest specific. (py is there internal helper library and iirc it originated from there.)

https://docs.pytest.org/en/stable/reference/reference.html#envvar-PY_COLORS

Signed-off-by: Stavros Ntentos <[email protected]>

Use the `_is_env_set_and_non_empty`, and the logic decided in the issue, to modify the reporter(s) list accordingly. At most, one reporter should be modified - the one going to `stdout`. Hooking before the `self.set_reporter` was deemed as the smallest incision. For no particular reason, it was decided that `_handle_force_color_no_color` would modify its input; so there would be no need for re-assigning the input we want to modify anyway. Signed-off-by: Stavros Ntentos <[email protected]>

Signed-off-by: Stavros Ntentos <[email protected]>

codecov · 2025-01-10T06:08:34Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.84%. Comparing base (14b242f) to head (4427cf3).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #9128   +/-   ##
=======================================
  Coverage   95.83%   95.84%           
=======================================
  Files         174      174           
  Lines       18995    19030   +35     
=======================================
+ Hits        18204    18239   +35     
  Misses        791      791

Files with missing lines	Coverage Δ
pylint/lint/pylinter.py	`96.84% <100.00%> (+0.19%)`	⬆️
pylint/lint/utils.py	`95.65% <100.00%> (+0.30%)`	⬆️
pylint/reporters/__init__.py	`100.00% <100.00%> (ø)`

pylint/lint/pylinter.py

stdedos · 2025-01-10T06:13:47Z

https://results.pre-commit.ci/run/github/47671127/1736488806.KxdAH7XbRZuDggnznvulyQ

mypy.....................................................................Failed
- hook id: mypy
- exit code: 1

tests/lint/test_pylinter.py:181: error: Argument 1 to "_handle_force_color_no_color" has incompatible type "list[TextReporter]"; expected "list[BaseReporter]"  [arg-type]
tests/lint/test_pylinter.py:181: note: "List" is invariant -- see https://mypy.readthedocs.io/en/stable/common_issues.html#variance
tests/lint/test_pylinter.py:181: note: Consider using "Sequence" instead, which is covariant
Found 1 error in 1 file (checked 278 source files)

If the suggestion is followed, I get:

$ pre-commit run -a
trim trailing whitespace.................................................Passed
fix end of files.........................................................Passed
ruff.....................................................................Passed
ruff-doc.................................................................Passed
copyright-notice.........................................................Passed
isort....................................................................Passed
black....................................................................Passed
black-doc................................................................Passed
black-disable-checker....................................................Passed
pylint...................................................................Failed
- hook id: pylint
- exit code: 1

Executable `pylint` not found

Check newsfragments......................................................Passed
rstcheck.................................................................Passed
mypy.....................................................................Failed
- hook id: mypy
- exit code: 1

pylint/lint/pylinter.py:307: error: "Sequence[BaseReporter]" has no attribute "pop"  [attr-defined]
pylint/lint/pylinter.py:308: error: "Sequence[BaseReporter]" has no attribute "append"  [attr-defined]
pylint/lint/pylinter.py:322: error: "Sequence[BaseReporter]" has no attribute "pop"  [attr-defined]
pylint/lint/pylinter.py:323: error: "Sequence[BaseReporter]" has no attribute "append"  [attr-defined]
Found 4 errors in 1 file (checked 278 source files)

prettier.................................................................Passed
pydocstringformatter.....................................................Passed
Check package with Pyroma................................................Passed
bandit...................................................................Passed
codespell................................................................Passed

I'm all ears for mypy/code improvement 🙌

for more information, see https://pre-commit.ci

DanielNoord

Left some comments that were a bit hard to explain as in line comments, hope you don't mind.

This LGTM expect for the small change in the news entry and the test

doc/whatsnew/fragments/3995.feature

pylint/lint/pylinter.py

DanielNoord · 2025-01-12T15:32:58Z

tests/lint/test_pylinter.py

+    [True, False],
+    ids=lambda force_color: f"{force_color=}",
+)
+def test_handle_force_color_no_color(


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.

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.

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

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?

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

stdedos · 2025-01-12T18:38:47Z

OTOH, @DanielNoord pls note that the latest changes you committed also makes pypy tests fail? Originally (https://github.com/pylint-dev/pylint/actions/runs/12704163354) they didn't

(Note to me: Historical runs: https://github.com/pylint-dev/pylint/actions/workflows/tests.yaml?query=branch%3Aimpr%2F3995%2FNO_COLOR%2BFORCE_COLOR)

for more information, see https://pre-commit.ci

DanielNoord · 2025-01-12T21:02:39Z

OTOH, @DanielNoord pls note that the latest changes you committed also makes pypy tests fail? Originally (https://github.com/pylint-dev/pylint/actions/runs/12704163354) they didn't

(Note to me: Historical runs: https://github.com/pylint-dev/pylint/actions/workflows/tests.yaml?query=branch%3Aimpr%2F3995%2FNO_COLOR%2BFORCE_COLOR)

Yeah, I wanted to spend time to fix the tests as well but would first like the opinion of Pierre about the current "design" of them.
I tried to refactor them a little just know but it is really hard. There are so many magic values and fixtures that it is really difficult (for me) to follow what is actually being done. I'm sure this is testing all edge cases described in the methods table, but I'm worried that this will be a nightmare to maintain.

I would probably prefer to not use "fancy" parameterisation for these tests, but instead write them out individually. In the long run I think that is better for maintainability.

Before committing to this I would like the opinion of another maintainer though.

Co-authored-by: Daniël van Noord <[email protected]>

Pierre-Sassoulas · 2025-01-13T21:09:06Z

This looks great, thank you for being persistent with this PR @stdedos !

github-actions · 2025-01-13T21:19:11Z

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit cd55074

Pierre-Sassoulas added the Enhancement ✨ Improvement to a component label Oct 7, 2023

This comment has been minimized.

Sign in to view

jacobtylerwalls changed the title ~~Take the FORCE_COLOR and NO_COLOR environnement variables into account~~ Take the FORCE_COLOR and NO_COLOR environement variables into account Oct 28, 2023

jacobtylerwalls changed the title ~~Take the FORCE_COLOR and NO_COLOR environement variables into account~~ Take the FORCE_COLOR and NO_COLOR environment variables into account Oct 28, 2023

This comment has been minimized.

Sign in to view

stdedos added 6 commits January 10, 2025 07:59

Create _is_env_set_and_non_empty

c6ca5e5

Signed-off-by: Stavros Ntentos <[email protected]>

Second option for _is_env_set_and_non_empty

aec5efb

test_handle_force_color_no_color

7e12334

towncrier: 3995.feature

7f38e76

Signed-off-by: Stavros Ntentos <[email protected]>

Documentation for pylint-dev#3995

4427cf3

Signed-off-by: Stavros Ntentos <[email protected]>

stdedos force-pushed the impr/3995/NO_COLOR+FORCE_COLOR branch from 51950fe to 4427cf3 Compare January 10, 2025 06:00

stdedos commented Jan 10, 2025

View reviewed changes

pylint/lint/pylinter.py Show resolved Hide resolved

This comment has been minimized.

Sign in to view

DanielNoord and others added 4 commits January 12, 2025 16:17

Remove PY_COLORS, fix spelling and don't modify in place

51cf8d2

[pre-commit.ci] auto fixes from pre-commit.com hooks

6d558c7

for more information, see https://pre-commit.ci

Follow up to fixes

8d8f988

Another follow up

bb87473

DanielNoord reviewed Jan 12, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

DanielNoord and others added 3 commits January 12, 2025 21:54

Make sure linting CI passes

68b07a4

[pre-commit.ci] auto fixes from pre-commit.com hooks

cf95d27

for more information, see https://pre-commit.ci

Rename to 'original_reporters'

c09eb18

This comment has been minimized.

Sign in to view

Update doc/whatsnew/fragments/3995.feature

cd55074

Co-authored-by: Daniël van Noord <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Take the `FORCE_COLOR` and `NO_COLOR` environment variables into account #9128

Take the `FORCE_COLOR` and `NO_COLOR` environment variables into account #9128

stdedos commented Oct 7, 2023

This comment has been minimized.

stdedos commented Oct 9, 2023

This comment has been minimized.

hugovk commented Jun 7, 2024 •

edited

Loading

This comment has been minimized.

codecov bot commented Jan 10, 2025

stdedos commented Jan 10, 2025 •

edited

Loading

This comment has been minimized.

DanielNoord left a comment

DanielNoord Jan 12, 2025

stdedos Jan 12, 2025

stdedos Jan 12, 2025

Pierre-Sassoulas Jan 13, 2025

DanielNoord Jan 14, 2025

This comment has been minimized.

stdedos commented Jan 12, 2025

DanielNoord commented Jan 12, 2025

This comment has been minimized.

Pierre-Sassoulas commented Jan 13, 2025

github-actions bot commented Jan 13, 2025

Take the FORCE_COLOR and NO_COLOR environment variables into account #9128

Are you sure you want to change the base?

Take the FORCE_COLOR and NO_COLOR environment variables into account #9128

Conversation

stdedos commented Oct 7, 2023

Type of Changes

Description

This comment has been minimized.

stdedos commented Oct 9, 2023

This comment has been minimized.

hugovk commented Jun 7, 2024 • edited Loading

This comment has been minimized.

codecov bot commented Jan 10, 2025

Codecov Report

stdedos commented Jan 10, 2025 • edited Loading

This comment has been minimized.

DanielNoord left a comment

Choose a reason for hiding this comment

DanielNoord Jan 12, 2025

Choose a reason for hiding this comment

stdedos Jan 12, 2025

Choose a reason for hiding this comment

stdedos Jan 12, 2025

Choose a reason for hiding this comment

Pierre-Sassoulas Jan 13, 2025

Choose a reason for hiding this comment

DanielNoord Jan 14, 2025

Choose a reason for hiding this comment

This comment has been minimized.

stdedos commented Jan 12, 2025

DanielNoord commented Jan 12, 2025

This comment has been minimized.

Pierre-Sassoulas commented Jan 13, 2025

github-actions bot commented Jan 13, 2025

Take the `FORCE_COLOR` and `NO_COLOR` environment variables into account #9128

Take the `FORCE_COLOR` and `NO_COLOR` environment variables into account #9128

hugovk commented Jun 7, 2024 •

edited

Loading

stdedos commented Jan 10, 2025 •

edited

Loading