Don't use color if NO_COLOR environment variable is set#1092
Don't use color if NO_COLOR environment variable is set#1092asottile merged 5 commits intopre-commit:masterfrom geieredgar:respect-no-color-environment-variable
Conversation
pre_commit/color.py
Outdated
| if setting not in COLOR_CHOICES: | ||
| raise InvalidColorSetting(setting) | ||
|
|
||
| if 'NO_COLOR' in os.environ: |
There was a problem hiding this comment.
this isn't quite right -- NO_COLOR= git commit ... should still have color
asottile
left a comment
asottile
left a comment
There was a problem hiding this comment.
looks good -- mind adding a quick test in tests/color_test.py -- you can use either mock.patch.dict(os.environ, ...) or pre_commit.envcontext.envcontext to test the various cases -- probably just needs these three cases considered:
- unset
- set to empty string
- set to non-empty string
pre_commit/color.py
Outdated
| raise InvalidColorSetting(setting) | ||
|
|
||
| if 'NO_COLOR' in os.environ: | ||
| if 'NO_COLOR' in os.environ and os.environ['NO_COLOR']: |
There was a problem hiding this comment.
if os.environ.get('NO_COLOR') would be ~slightly simpler
There was a problem hiding this comment.
Ok, I will change that.
|
Ok, I will do that |
tests/color_test.py
Outdated
|
|
||
|
|
||
| def test_no_color_env_unset(): | ||
| with mock.patch.dict(os.environ): |
There was a problem hiding this comment.
clear=True is what you want here I believe
There was a problem hiding this comment.
Wouldn't that remove all entries from os.environ? It would probably still work, though.
There was a problem hiding this comment.
yes, and yes
I guess if you want to be more direct you could use:
with envcontext([('NO_COLOR', UNSET)]):
...envcotext and UNSET come from pre_commit.envcontext
There was a problem hiding this comment.
Ok, I will change that because otherwise the coverage is reduced.
|
thanks again! and thanks @saper for the issue! |
2 participants
Resolves #1073