-
-
Notifications
You must be signed in to change notification settings - Fork 767
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
Don't use color if NO_COLOR environment variable is set #1092
Don't use color if NO_COLOR environment variable is set #1092
Conversation
pre_commit/color.py
Outdated
@@ -48,6 +48,9 @@ def use_color(setting): | |||
if setting not in COLOR_CHOICES: | |||
raise InvalidColorSetting(setting) | |||
|
|||
if 'NO_COLOR' in os.environ: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this isn't quite right -- NO_COLOR= git commit ...
should still have color
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@asottile Ok, I pushed a new commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -48,7 +48,7 @@ def use_color(setting): | |||
if setting not in COLOR_CHOICES: | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if os.environ.get('NO_COLOR')
would be ~slightly simpler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clear=True
is what you want here I believe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't that remove all entries from os.environ
? It would probably still work, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I will change that because otherwise the coverage is reduced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks again! and thanks @saper for the issue! |
Resolves #1073