Remove "no-colors" option #275

Closed
markelog opened this Issue Feb 26, 2014 · 18 comments

8 participants

@markelog
jscs-dev member

It's duplicate and needless, this matters should be deal with through tty module.

It would be a incompatible change, so it should be done in major release.

@markelog markelog added the cli label Feb 26, 2014
@vtambourine

What makes you think so?

That option can be very useful in various CI log outputs.

@markelog
jscs-dev member

@vtambourine

That option can be very useful in various CI log outputs.

see

this matters should be deal with through tty module

If terminal has the ability to display colors it would do so, we just need to check that, we could also check for the specific env var.

@sindresorhus
jscs-dev member

I'm gonna shamelessly suggest switching to Chalk.
Colors is bad since it extends the String.prototype, which affects all loaded modules.

Chalk is able to detect color support and TTY and includes support for overriding with the --color and --no-color flags, so you don't have to care.

@joshuaspence

-1, --no-colors is useful.

@mikesherov

How does this work when I want to pipe the output to something like grep while still using a color supported terminal? Will it know to not use colors?

@sindresorhus
jscs-dev member
@zarkone

I've added --no-colors because i had no ability to work in dumb terminals. From my point of view, we can just check color support in current terminal to decide how to run jscs -- with colors support or without it. Replace colors to Chalk or whatever could become a reason of bugs or undefined behaviour -- i suggest to make such decision if only there are some more strong reasons to do such migration. And even if migration would be approved i believe that it's better to leave the ability to force monochrome mode -- at least explain in docs how to switch mode with $TERM variable.

@markelog
jscs-dev member

Right now i'm thinking to switch to chalk and leave the option, but that would mean that user could not force colors output (if such option would be requested) if terminal does not support it.

How is that sounds?

@markelog
jscs-dev member

Oh, i see chalk.enabled option, cool, switching than

@markelog markelog added a commit that closed this issue Jul 23, 2014
@markelog markelog Use chalk module instead of colors
It also detects tty, so we do not need to

Fixes #275
76f372c
@markelog markelog closed this in 76f372c Jul 23, 2014
@markelog
jscs-dev member

Hm, apparently i speak to soon, our reporters can decide if they want color in their output, they do that by passing an additional argument to explainError method.

But in order to preserve current logic, we would need to change chalk.enabled value every time explainError method is called to the value of that argument, which mean it would have a side effect which it did not have before, since it would change state of chalk module.

We shouldn't do that.

@sindresorhus is that possible to disable auto-detection of tty? Would you accept a PR for that?

@markelog
jscs-dev member

Alrighty than, reverting

@sindresorhus
jscs-dev member

which mean it would have a side effect which it did not have before, since it would change state of chalk module.

What side-effect? Sorry I'm not aware of how jscs works internally at all.

is that possible to disable auto-detection of tty?

Why would you need that when you have chalk.enabled?

@necolas

Will you be removing the "Will be removed" statement in the docs for --no-colors?

@markelog
jscs-dev member

See c57be47

@necolas

Fast! :)

@lahmatiy lahmatiy added a commit that referenced this issue Dec 16, 2014
@markelog markelog Use chalk module instead of colors
It also detects tty, so we do not need to

Fixes #275
feca4d9
@jbnicolai

@markelog @sindresorhus `Chalk 1.0.0 has just been released.

is that possible to disable auto-detection of tty? Would you accept a PR for that?

This is now possible. See this line from the release notes:

  • Add ability to force color by setting the FORCE_COLOR environment variable.

And the second, previous, issue:

But in order to preserve current logic, we would need to change chalk.enabled value every time explainError method is called to the value of that argument, which mean it would have a side effect which it did not have before, since it would change state of chalk module.

This too has now been fixed! Rather than globally enabling and disabling the chalk module, it can now be set on a per-instance basis:

  • Ability to disable/enable colors on a per instance basis, rather than globally.

Would you be interested in picking up the move to chalk again?

@mikesherov

@jbnicolai, I'd be interested if there was a pull request re-using it. What we have now works, but would love to be able to offload that to chalk, it's just that the core team at the moment has other priorities... So yeah, if there's a PR to reuse chalk, we'd love to use it :-)

@jbnicolai

@mikesherov I'll see what I can do :)

@mikesherov mikesherov added a commit that referenced this issue Mar 2, 2015
Joshua Appelman Replaces the 'colors' and 'supports-colors' packages with 'chalk'.
Refs #275
Closes gh-1118
a9e7f09
@lahmatiy lahmatiy added a commit that referenced this issue Mar 28, 2015
Joshua Appelman Replaces the 'colors' and 'supports-colors' packages with 'chalk'.
Refs #275
Closes gh-1118
18c0abd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment