Fix NO_COLOR handling to comply with the spec

[chocolateboy]

Closes #38

disable color with NO_COLOR:

before:

$ NO_COLOR=1 command  # ✔
$ NO_COLOR=0 command  # ✔
$ NO_COLOR= command   # x
$ NO_COLOR="" command # x

after:

$ NO_COLOR=1 command  # ✔
$ NO_COLOR=0 command  # ✔
$ NO_COLOR= command   # ✔
$ NO_COLOR="" command # ✔

also:

• don't equate FORCE_COLOR="" with FORCE_COLOR=1
• add environment-variable tests (based on test/env.sh)

Note that the 4 tests which combine NO_COLOR and FORCE_COLOR fail on Node.js >= v12 because it has elected to issue a warning if both are defined.

[codecov-commenter]

Codecov Report

Merging #40 into master will decrease coverage by 1.16%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #40      +/-   ##
==========================================
- Coverage   98.77%   97.60%   -1.17%     
==========================================
  Files           2        2              
  Lines         163      167       +4     
  Branches       31       32       +1     
==========================================
+ Hits          161      163       +2     
- Misses          1        3       +2     
  Partials        1        1              

Impacted Files	Coverage Δ
colors.mjs	96.36% <100.00%> (-1.75%)	⬇️
index.mjs	98.21% <100.00%> (-0.88%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b08ad63...a5c6a2a. Read the comment docs.

[lukeed]

Hey, FORCE_COLOR logic is the same as chalk's support-color so that has to stay. It's the same reasoning as NO_COLOR, apparently.

Also, I really appreciate you taking time to modify tests, but I'm very hesitant of moving the ENV tests to something else entirely. Do you mind reverting those changes and just adding new test cases to the bash file there?

Thank you

[lukeed]

I went with #39 for the code fix because it arrived first, but definitely looking out for your test additions!

[chocolateboy]

FORCE_COLOR logic is the same as chalk's support-color

I don't think it is, but then chalk's "logic" makes very little sense to me either way.

I'm very hesitant of moving the ENV tests to something else entirely. Do you mind reverting those changes and just adding new test cases to the bash file there?

I restored your env.sh since it's required by the CI. I initially added the tests to it, but I couldn't see the point of adding tests to a non-test file. The Bats test doesn't add any dependencies that aren't already required by env.sh (i.e. node and bash) and IMO is much easier to read/maintain. (Also, env.sh doesn't expose the warning about the NO_COLOR/FORCE_COLOR conflict.)

Anyway, closing this since the other PR fixes the NO_COLOR issue.

[lukeed]

It's the same through this line: https://github.com/chalk/supports-color/blob/c5edf46896d1fc1826cb1183a60d61eecb65d749/index.js#L27 An empty string will force it to be 1

I agree it doesn't make sense but we're stuck respecting someone else's decisions -- just like NO_COLOR

Do you plan on adding the bash changes?
I appreciate the idea behind bats, but honestly it's something I've never seen before and realistically won't see again. I don't want to take on something that will force me to learn it just so that I can add or tweak something down the road.

[chocolateboy]

It's the same through this line

FORCE_COLOR=false doesn't disable colors and FORCE_COLOR=2 doesn't enable more colors. (And that's not the full extent of chalk's quirks.)

I appreciate the idea behind bats, but honestly it's something I've never seen before and realistically won't see again. I don't want to take on something that will force me to learn it just so that I can add or tweak something down the road.

I don't think there's anything to learn, really. It's declarative, and IMO much clearer than env.sh, which was producing mangled output (presumably caused by the warnings). Fixing that would have required me to dive into bash scripting, and — like I say — I couldn't see the point in Stack Overflowing enough bash to fix a manual test when the test cases could easily be added to an automated test.

If you know bash, then you know Bats. If you don't know bash, Bats is the best way to avoid it :-)