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

reporters/base: use supports-color to detect colorable term #1545

Merged
merged 1 commit into from
Feb 12, 2015
Merged

reporters/base: use supports-color to detect colorable term #1545

merged 1 commit into from
Feb 12, 2015

Conversation

parkr
Copy link
Contributor

@parkr parkr commented Feb 10, 2015

This replaces #1497.

Detecting whether a terminal is TTY is not enough
to ensure that it can also support color.
This module, supports-color, is a wholistic solution
which captures the greatest range of possible terminals
with the greatest accuracy. The module is extremely
lightweight and does not represent a burdern to Mocha
users.

@dasilvacontin
Copy link
Contributor

LGTM if we fix the build errors.

@jbnicolai
Copy link

Looks like the problem is this line. Seems supports-color chose to only support node >= 10.0, probably just to encourage adoption of a more modern node in the ecosystem.

I'll see if I can have it support node >= 8.0.0.

@parkr
Copy link
Contributor Author

parkr commented Feb 10, 2015

I think @dasilvacontin is right – the ^ character is crazy. I remember being able to use ~ in older versions of node. My guess is this new CL will work.

@dasilvacontin
Copy link
Contributor

I would prefer using the caret character. Can't we just force a higher version of npm? Kinda what we ended up doing at mustache.js: janl/mustache.js#421

@parkr
Copy link
Contributor Author

parkr commented Feb 10, 2015

I would prefer using the caret character.

What do you prefer about the caret character?

Can't we just force a higher version of npm? Kinda what we ended up doing at mustache.js: janl/mustache.js#421

Force-install a newer version of NPM? That's doable as long as it's compatible with that version of node. When was the caret character introduced?

@dasilvacontin
Copy link
Contributor

rlidwka/sinopia#49 (comment)

With the caret character, we will be getting both minor releases and patches/bugfixes. With the tilde, as soon as they do either a major or a minor release we will stop automatically getting patches and bugfixes.

On a second look, it seems like the philosophy in mocha is manually updating dependencies, since a lot of people rely on this package for their tests.

@@ -33,7 +34,7 @@ exports = module.exports = Base;
* Enable coloring by default.
*/

exports.useColors = isatty || (process.env.MOCHA_COLORS !== undefined);
exports.useColors = (isatty && supportsColor) || (process.env.MOCHA_COLORS !== undefined);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

supportsColor already checks if stdout is TTY, so I think the isatty check could be dropped.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected.

This replaces #1497.

Detecting whether a terminal is TTY is not enough
to ensure that it can also support color.
This module, supports-color, is a wholistic solution
which captures the greatest range of possible terminals
with the greatest accuracy. The module is extremely
lightweight and does not represent a burdern to Mocha
users.
@sindresorhus
Copy link
Contributor

Any reason you're not just using chalk?

@jbnicolai
Copy link

@sindresorhus discussed in #1330 and #1325. I'm still all in favor, especially considering we already use supports-color and escape-string-regexp, both of which chalk wraps.

@boneskull
Copy link
Contributor

@sindresorhus, @jbnicolai, @dasilvacontin

Re: fuzzy dependencies (^ vs ~) --fine for devDependencies. We should keep them exact for production dependencies, however.

Would prefer to use chalk or something. Someone just needs to implement it

@parkr
Copy link
Contributor Author

parkr commented Feb 12, 2015

Ok, this is starting to feel like a wild 🐦 chase. Chalk looks like a pretty big lift – completely reworking the way writing to the console happens. Am I correct in this assumption?

@travisjeffery
Copy link
Contributor

@parkr yep, gonna merge this and we can talk about chalk somewhere else if necessary

travisjeffery pushed a commit that referenced this pull request Feb 12, 2015
reporters/base: use supports-color to detect colorable term
@travisjeffery travisjeffery merged commit 4dbf640 into mochajs:master Feb 12, 2015
@parkr parkr deleted the supports-color branch February 12, 2015 06:14
@parkr
Copy link
Contributor Author

parkr commented Feb 12, 2015

Thank you!

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.

6 participants