Fix handling empty values of NO_COLOR and FORCE_COLOR #11712
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fix handling NO_COLOR and FORCE_COLOR environment variables to correctly be ignored when they are set to an empty value, as defined in the specification: > Command-line software which adds ANSI color to its output by default > should check for a NO_COLOR environment variable that, when present > *and not an empty string* (regardless of its value), prevents > the addition of ANSI color. (emphasis mine, https://no-color.org/) The same is true of FORCE_COLOR, https://force-color.org/.
mgorny
force-pushed
the
no-color-correct
branch
from
ac007d3
to
4a18fc3
Compare
testing/io/test_terminalwriter.py
Outdated
| @@ -213,6 +213,12 @@ def test_should_not_do_markup_NO_COLOR_and_FORCE_COLOR( | |||
| assert_color_not_set() | |||
|
|
|||
|
|
|||
| def test_empty_NO_COLOR_ignored(monkeypatch: MonkeyPatch) -> None: | |||
| monkeypatch.setitem(os.environ, "NO_COLOR", "") | |||
| monkeypatch.setitem(os.environ, "FORCE_COLOR", "1") | |||
There was a problem hiding this comment.
When setting both,the test doesn't actually cover all cases plus off hand I don't recall if our checks consider externally set variables
There was a problem hiding this comment.
I'm sorry but I don't understand the second part of your sentence. Could you rephrase, please?
Streamline the tests for FORCE_COLOR and NO_COLOR variables, and cover all possible cases (unset, set to empty, set to "1"). Combine the two assert functions into one taking boolean parameters. Mock file.isatty in all circumstances to ensure that the environment variables take precedence over the fallback value resulting from isatty check (or that the fallback is actually used, in the case of both FORCE_COLOR and NO_COLOR being unset).
|
I've done some refactoring and covered all cases of |
|
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix handling NO_COLOR and FORCE_COLOR environment variables to correctly be ignored when they are set to an empty value, as defined in the specification:
(emphasis mine, https://no-color.org/)
The same is true of FORCE_COLOR, https://force-color.org/.
If this change fixes an issue, please:
closes #XYZWto the PR description and/or commits (whereXYZWis the issue number). See the github docs for more information.Unless your change is trivial or a small documentation fix (e.g., a typo or reword of a small section) please:
Create a new changelog file in the
changelogfolder, with a name like<ISSUE NUMBER>.<TYPE>.rst. See changelog/README.rst for details.Write sentences in the past or present tense, examples:
Also make sure to end the sentence with a
..Add yourself to
AUTHORSin alphabetical order.