-
Notifications
You must be signed in to change notification settings - Fork 51
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
FORCE_COLOR=0 forces color #43
Comments
Thank you, @chocolateboy. I recently added tests for FORCE_COLOR and NO_COLOR. Could you explain why I thought I had understood the issue, but I think I lost it again. My understanding is that FORCE_COLOR={1,2,3...} forces color and that FORCE_COLOR={,0} should do nothing (as opposed to deliberately disable color). |
To answer my own question, I think it is because when we pipe colorette, color should be disabled. |
The "no color" in those cases is because of the pipe. The |
🎉 @chocolateboy I got it! But... Now that I finally understand the issue, I'd like to advocate against implementing
It's also easier to reason about Now, I get it, maybe you have a script where you set Here are two ways to do it that come to mind:
set -l COLOR_MODE = NO_COLOR # or FORCE_COLOR
env $COLOR_MODE= ./test.js or a one-liner: env (test $condition && echo FORCE_COLOR || echo NO_COLOR)= ./test.js |
I think this is an awful lot of work to avoid adding a few bytes to the source code :-/ As you mention, the behavior of
I think it's easy enough to reason about the semantics if the code is written clearly, which is why I ungolfed it. Either way, as mentioned before:
|
@chocolateboy I respect your opinion, but I don't consider that to be a lot of work. And that's only work you'd do only if you wanted to switch between Frankly, I don't care about the bytes, this is Node after all—your node_modules are heavier than the universe. My reason to go against this is not bytes. I just find things like I don't think there's a standard governing any of this, so I wouldn't claim the behavior of
My eyes!!! 👀🩸 When I first encountered chalk way back in the day, I was frustrated by its complexity. Today, it's even more complex. It's complex any way you look at it. Some people are more adept at dealing with complexity. Others don't care. I am very particular about this. I just want colorette to be simple, whether you are using it or looking under the covers. It was probably already a bad idea enabling/disabling color out of the box, but I can't roll that back now. |
AFAICT, no other JS implementations of |
@chocolateboy What if we could do better than them? By the way, do you have an example of |
I don't think "everyone else is doing it wrong - let's innovate!" is a better approach.
My original use case is here:
If you mean an example of how it's interpreted in Node.js libraries/apps, see here. I couldn't find an example in the first 10 pages which behaves in the way you're proposing (i.e. like |
Hey sorry to comment on an old issue, but Node.js itself now disables color then It'd be great for JS libraries to align with the Node.js behavior. |
Thanks for the heads-up, @nicolo-ribaudo! 💯 |
Re-opening this to ensure it doesn't get overlooked. Test case copied from #41 (comment). Fixed in #41, but the patch was modified, which introduced the new bug.
In the current version (1.2.0) of Colorette, the following all have the same effect:
FORCE_COLOR=1
FORCE_COLOR=0
FORCE_COLOR=
i.e.
FORCE_COLOR=<falsey/no value>
incorrectly forces color which wouldn't otherwise be displayed.test.js
1) expect color
$ FORCE_COLOR=1 node ./test.js | strings
output ✔
2) expect no color (because of the pipe)
$ node ./test.js | strings
output ✔
3) expect no color (same as 2)
$ FORCE_COLOR=0 node ./test.js | strings
output ❌ 🐛
4) expect no color (same as 2)
$ FORCE_COLOR= node ./test.js | strings
output ❌ 🐛
The text was updated successfully, but these errors were encountered: