Skip to content
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

Fix NO_COLOR support #39

Merged
merged 3 commits into from
Sep 26, 2020
Merged

Fix NO_COLOR support #39

merged 3 commits into from
Sep 26, 2020

Conversation

stramel
Copy link
Contributor

@stramel stramel commented Sep 25, 2020

Fixes #38

Checks that NO_COLOR is present to disable the colors. Sorry for missing this in the spec the first time around.

@lukeed It might be nice to share this enabled logic between the 2 implementations. I'm unsure how that would affect the bench for kleur.

Also, I noticed that env.sh isn't running for tests. I had troubles with getting it to fail with the old logic or the new logic. I instead ran NO_COLOR= yarn test to see if it was disabling the colors and it appeared to work.

@codecov-commenter
Copy link

codecov-commenter commented Sep 25, 2020

Codecov Report

Merging #39 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #39   +/-   ##
=======================================
  Coverage   98.77%   98.77%           
=======================================
  Files           2        2           
  Lines         163      163           
  Branches       31       29    -2     
=======================================
  Hits          161      161           
  Misses          1        1           
  Partials        1        1           
Impacted Files Coverage Δ
colors.mjs 98.11% <100.00%> (ø)
index.mjs 99.09% <100.00%> (ø)

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...d85c880. Read the comment docs.

@lukeed
Copy link
Owner

lukeed commented Sep 26, 2020

It might be nice to share this enabled logic between the 2 implementations. I'm unsure how that would affect the bench for kleur

By "2 implementations" do you mean kleur and kleur/colors? If so, definitely agree!
Won't affect bench at all. Only evaluates once as part of setup.

I have the bash script I call locally. But it's run automatically as part of VI

@lukeed lukeed merged commit ef5d4a8 into lukeed:master Sep 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NO_COLOR implementation doesn't match the spec
3 participants