-
-
Notifications
You must be signed in to change notification settings - Fork 85
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
Add ability to force disable colors with an environment variable #31
Conversation
Wouldn't it be better to have a separate variable, like |
I read |
I'd advice not to use generic variable names like that. Something like |
|
Are the any plans to merge this pull request yet? Need this with regards to issues with PowerShell on Windows displaying Gulp output (using chalk). Need to disable coloring to workaround that issue. Checking on the value for FORCE_COLOR like FORCE_COLOR=0 will work from my opinion. Any ETA on releasing of this change? |
I think this PR looks fine and wouldn't mind merging it. @sindresorhus should have the final word. |
I thought #32 superseded this? |
Perhaps at one point. This could stand on its own, though. This seems more sane than introducing a new env variable. I would personally want to see this one before I saw the other one, and I think the other one could use some improvement whereas this one is good to go. |
I agree that no need to introduce the new variable right now. So, any ETA on merging? |
Actually I don't think this should be merged, as this introduces a new behavior which is unique to this package. Is there any documentation for the I think a new variable might be best, maybe |
To my knowledge the value for FORCE_COLOR is ignored, so I don't think anybody is using this one. But we need to rely on value instead of just existence of the variable in order to do the "switch". Introduction of the new variable will add a lot of complexity to "sync" all those variables' values. |
@Qix- Apologies, bit late to the discussion, but I'm with @sindresorhus that this is confusing. I interpreted (and initially implemented) FORCE_COLOR as a boolean flag, i.e.: override everything, or just apply default behaviour. I agree that similar behaviour like FORCE_NO_COLOR also has it's usecases, but I think separating that into two boolean flags (force-go, force-no) is easier. Since the original flag name 'FORCE_COLOR' was chosen simply because we weren't aware for a better standard, I'm leaning towards #32 as well, and suggest we close this one. If there's a better standard than suggested in #32, let's discuss there. Thanks for the effort though! |
Totally disagree. Having two environment variables makes no sense. What happens in the following scenario? The outcome is unclear. FORCE_NO_COLOR= FORCE_COLOR= some-ansi-program --color whereas FORCE_COLOR= some-ansi-program
# or
FORCE_COLOR=1 some-ansi-program and FORCE_COLOR=0 some-ansi-program make perfect sense. This is also analogous to other programs I've seen using environment variables as boolean values (namely, Environment variables shouldn't be used as a programming construct, they should be used as configurations. Throwing more crap into your environment only waters things down and I firmly believe in the long run it'll confuse people to have two variables. With one, all we need in the docs is With two, we'll instead need a whole truth table:
|
I don't think this is a problem as there are cases like this anyway:
Meaning that we have unclear scenarios anyway, a new environment variable would just introduce some more :D
It would not be intuitive to me that this would never output color. |
But it's clear that FORCE means trump everything. It's clear from our docs and it's clear from context. Using FORCE_COLOR=0 with --color is pretty clear as to what happens. |
True. Okay you convinced me: I'm against adding a new env var. Still not convinced about
(as implemented by #34) makes more sense. |
Im all for renaming it whatever if it doesn't make sense. Functionally though I remain of the opinion it should be one var. |
Unfortunately one (boolean) variable doesn't suffice, as there are 3 cases: on, off, auto. The last case could be handled by the |
Shouldn't rely on the As for the |
That wouldn't work on Window's cmd.exe, since it doesn't support ANSI colors out of the box. |
@jhasse it works just fine now, what are you talking about? By default Windows supports ANSI escapes thanks to LibUV, obviously only within the Node.js environment. Barring renaming this variable, this is how it would work:
It literally cannot get any simpler than that. This is a cross platform way to do it. |
@Qix- As you said yourself this is only within the Node.js environment. I was hoping for an universal variable name, and the Mac tools are already using CLICOLOR(_FORCE). |
|
Than it still stands that outside of the Node.js environment, cmd.exe doesn't support ANSI escape codes out of the box. Therefore programs shouldn't output them when the variable isn't defined. |
@jhasse I'm not sure what you mean by that. Will |
No. I'm looking for variable names which wouldn't only be used by |
I'm still a 'yes' for this, after giving it some time @sindresorhus. I've run into a multitude of situations where Any more discussion on this is bikeshedding - if I'm defining environment variables just to exist I'm not going to be setting them to |
+1 In order to keep variable names which control ANSI colors to a minimum I still hope for |
You said it yourself. I'm still on board with this PR and would like to see some movement on it. I'm tempted to merge this PR at the moment since we've been sitting on it for so long and it's becoming a point of bike-shed at this point. |
supportLevel = 1; | ||
if ('FORCE_COLOR' in process.env) { | ||
var forceColor = parseInt(process.env.FORCE_COLOR, 10); | ||
supportLevel = forceColor === 0 ? 0 : supportLevel || 1; |
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.
Wrap supportLevel || 1
in parens to make the precedence clear.
@@ -44,7 +44,7 @@ The returned object specifies a level of support for color through a `.level` pr | |||
|
|||
It obeys the `--color` and `--no-color` CLI flags. | |||
|
|||
For situations where using `--color` is not possible, add an environment variable `FORCE_COLOR` with any value to force color. Trumps `--no-color`. | |||
For situations where using `--color` is not possible, add an environment variable `FORCE_COLOR` with any value to force color. If `FORCE_COLOR` is set to `0`, colors are force disabled. Trumps `--no-color` / color flags, respectively. |
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.
I think we should drop with any value
, and instead say FORCE_COLOR=1
to force color, FORCE_COLOR=0
to force disable colors, and don't use it for auto-detection (for clarity).
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.
I've re-worded that whole section.
var result = requireUncached('./'); | ||
assert.equal(Boolean(result), true); | ||
assert.equal(result.level, 2); | ||
}); |
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.
For completeness, can you add a test for process.env.FORCE_COLOR = '1';
too?
I've been thinking long and hard about this issue and I've decided to go with this solution. I really don't want to introduce another environment variable, and from looking through the proposals about |
@sindresorhus the requested changes have been made. |
So this extends the
FORCE_COLOR
environment variable to allowFORCE_COLOR=0
(preserving the behavior of justFORCE_COLOR=
), which will forcibly disable color environment-wide.This was inspired by the bug over at electron/electron#1647