Repository navigation
gh-129061: Fix FORCE_COLOR and NO_COLOR when empty strings - #129140
hugovk merged 8 commits into
Conversation
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
🤖 New build scheduled with the buildbot fleet by @hugovk for commit ec36e22 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F30617%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
Sorry, something went wrong.
|
Like #129137 (comment), test_external_inspection failed on Fedora and RHEL8 buildbots and is unrelated. |
Sorry, something went wrong.
serhiy-storchaka
left a comment
There was a problem hiding this comment.
We only need to test pairs of environment variables that set conflicting values.
For example, NO_COLOR=1 and TERM=dumb should be tested with FORCE_COLOR=1 and PYTHON_COLOR=1. Tests for a single environment variable which forces color (FORCE_COLOR and PYTHON_COLOR) should be performed when the fallback value determined by isatty or _supports_virtual_terminal is False.
We need also to test can_colorize() when stderr is a TTY, but stdout is not, and with an explicit file argument. Including corner cases: no file.fileno, no file.isatty, file.isatty() and os.isatty(file.fileno()) return different results. BTW, I think that file.isatty() should perhaps be tested before os.isatty(file.fileno()).
This is a large issue and can be done in a separate PR.
Sorry, something went wrong.
|
Some failures on Windows for: ({"PYTHON_COLORS": "true"}, True),
({"PYTHON_COLORS": "2"}, True),
({"PYTHON_COLORS": ""}, True),That's because according to the docs:
Therefore everything that is not "0" or "1" is ignored, and the result depends on other conditions. This also fails on Windows: ({"NO_COLOR": ""}, True),That's because an empty value means we ignore it, and again the result depends on other conditions. Shall we remove these four, or use them in combination with some other variables? |
Sorry, something went wrong.
|
See improved tests in #129234. They cover all corner cases. I propose to push it first, and then add tests for empty values. |
Sorry, something went wrong.
serhiy-storchaka
left a comment
There was a problem hiding this comment.
Only the first two checks are necessary. Others do not add anything useful. They look out of place and make the intention of the tests less clear.
The first group tests the different values of the variables. The second group tests relative priority of the variables (it does not repeat tests for different values). Then the same is done for PYTHON_COLORS -- first the different values of PYTHON_COLORS (including the empty value), then its relative priority to other variables.
Sorry, something went wrong.
|
Okay, I've removed all but the first two. |
Sorry, something went wrong.
serhiy-storchaka
left a comment
There was a problem hiding this comment.
LGTM! 👍
Sorry, something went wrong.
|
Thanks @hugovk for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13. |
Sorry, something went wrong.
…ythonGH-129140) (cherry picked from commit 9546fe2) Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
|
GH-129360 is a backport of this pull request to the 3.13 branch. |
Sorry, something went wrong.
…GH-129140) (#129360) gh-129061: Fix `FORCE_COLOR` and `NO_COLOR` when empty strings (GH-129140) (cherry picked from commit 9546fe2) Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
First refactor the tests into subtests, which makes it easier compare the env var inputs and expected outputs.