Added support for --no-color flag in verbose and mini reporters #1198

ThomasBem · 2017-01-18T18:10:44Z

Started working on fixing #843. So far I have managed to disable color output for both mini and verbose reporters by setting the --no-color flag.

Now I need to disable color output also if you try console.log with colors inside a test. What I am a little stuck on at the moment is this spawning of child processes. Thats a part of the code base I have yet to understand.

Any pointers on where to start digging? :)

novemberborn · 2017-01-19T11:23:59Z

Now I need to disable color output also if you try console.log with colors inside a test

That seems unnecessary. If you want to log colors then go ahead. It's just that one should be able to disable AVA's color output.

novemberborn · 2017-01-19T11:22:27Z

lib/cli.js

@@ -124,9 +125,14 @@ exports.run = () => {
 	if (cli.flags.tap && !cli.flags.watch) {
 		reporter = tapReporter();
 	} else if (cli.flags.verbose || isCi) {
-		reporter = verboseReporter();
+		reporter = verboseReporter({
+			disableColors: cli.flags.color !== undefined


Without looking into meow, I'm surprised this isn't a boolean. Does it become false if --no-color is set? Comparing against false seems better than comparing against undefined.

@novemberborn By default, if the user doesn't specify either --color or --no-color, it's not set.

If we add the following, it will be true if not specified, but then we wouldn't know when it's not specified and should be auto-detected. So I wouldn't do this:

const cli = meow(``, { boolean: ['color'], default: { color: true } });

I would go with color instead of disableColors though. Double-negatives are not very good.

I will double check this. Was stuck on this point for a while trying out different things and reading up on meow and minimist. Got it working on undefined, but maybe I missed something.

@sindresorhus I tried to do what you suggested with default in in meow and that works for my code,
{ color: true }
but I dont understand whats going on with whats currently in default and how to make it work together with the change you recommended. Tried a few different things, but I keep getting a failing test in cli.js.

https://github.com/avajs/ava/blob/master/lib/cli.js#L75

novemberborn · 2017-01-19T11:22:55Z

lib/reporters/mini.js

@@ -28,6 +28,13 @@ function MiniReporter(options) {

 	this.options = Object.assign({}, options);

+	if (this.options.disableColors) {
+		chalk.enabled = false;
+		Object.keys(colors).forEach(function (key) {


I'd personally prefer a for…of loop here.

I just re-used the existing code that in other places did this but with "=true". But can make changes if you think that`s better.

https://github.com/ThomasBem/ava/blob/7c00c13844da6ab116676027b6482a57cf0f102a/lib/reporters/verbose.js#L9:L11

https://github.com/ThomasBem/ava/blob/7c00c13844da6ab116676027b6482a57cf0f102a/lib/reporters/mini.js#L14:L16

Yes, go with for-of whenever possible.

ThomasBem · 2017-01-19T18:30:18Z

@sindresorhus commented this in one of the PR`s that was working on this issue previously.

It also needs to pass the --no-color flag down to the forked process. If I add console.log(require('chalk').red('colorzzzz')); in a test file, it will still log colors even with $ ava --no-colors. I would expect that to prevent all colors.

#1104

novemberborn · 2017-01-20T11:40:57Z

@ThomasBem I guess I disagree with @sindresorhus then 😄

@sindresorhus why do you think ava --no-color should disable colors not coming from AVA itself?

sindresorhus · 2017-01-23T12:02:50Z

why do you think ava --no-color should disable colors not coming from AVA itself?

Because that's how it would work normally. That's how I designed Chalk. AVA is special since we spawn child processes, but the expected behavior should be preserved, by passing down the flag. --no-color should apply to everything.

novemberborn · 2017-01-23T12:46:43Z

Ah, hadn't realized chalk takes a --no-color argument.

We've previously argued against forwarding command line arguments to the workers though.

sindresorhus · 2017-01-23T13:04:24Z

We've previously argued against forwarding command line arguments to the workers though.

Yes, and we're not gonna start doing it with arbitrary flags, but I would say it's worth making an exception for this one as it matches expected outcome.

ThomasBem · 2017-01-24T18:55:49Z

I have updated the PR with the suggested changes regarding the CLI flag and the for...of loops.

I need some help with keeping the existing "default: conf" in cli.js while also adding the new stuff @sindresorhus mentioned. I could not get it to work together with the new { color: true} so I just removed it for now and its causing a test to fail.

I don`t understand what this conf thing is and what its doing in default in the first place. It makes no sense to me :P

I can understand adding { color: true } saying that color should be true if nothing else is set. But just "conf" makes no sense to me.

I also need some support in where to start with passing the flag down to the child processes. That`s one of the things I have yet to understand with how Ava works :)

novemberborn · 2017-01-25T10:33:09Z

I don`t understand what this conf thing is and what its doing in default in the first place. It makes no sense to me :P

This may be relevant: #1046 (comment)

I could not get it to work together with the new { color: true} so I just removed it for now and its causing a test to fail.

The test verifies that config in package.json overrides the default behavior. Currently the config is passed through the CLI parser (meow) using default, except that you removed that behavior 😉

If you do Object.assign({ color: true }, conf) instead you should get the behavior you want, and a passing test. I think this is another argument for my suggestion in #1046 (comment).

I also need some support in where to start with passing the flag down to the child processes. That`s one of the things I have yet to understand with how Ava works :)

See here:

ava/lib/fork.js

Line 39 in 872d2ed

    
           const ps = childProcess.fork(path.join(__dirname, 'test-worker.js'), [JSON.stringify(opts)], {

opts should already contain the colors value, you'll just have to translate it to the correct flag.

ThomasBem · 2017-01-25T19:43:16Z

Thanks for the help @novemberborn :)

I did what you said for the default, and that worked fine. Think I sort of understand whats going on with that now.

I also made something that works for passing the color flag down to the forked process.

I am unsure of how / if I can test that small piece of code though. Any suggestions?

forresst · 2017-01-26T10:39:29Z

lib/cli.js

@@ -38,6 +38,7 @@ exports.run = () => {
 		  --verbose, -v           Enable verbose output
 		  --no-cache              Disable the transpiler cache
 		  --no-power-assert       Disable Power Assert
+		  --no-color              Disable color output


This flag should be added to readme.md (like -no-power-assert for example) in CLI paragraph

Good catch! Did not even think about that.

Should be good now :)

forresst · 2017-01-26T14:22:26Z

readme.md

@@ -159,6 +159,7 @@ $ ava --help
    --verbose, -v           Enable verbose output
    --no-cache              Disable the transpiler cache
    --no-power-assert       Disable Power Assert
+	--no-color              Disable color output


Please, fix indentation

Hmm, ill take a look. It does not look that way for me in IDE or when looking at the readme in my branch. I see even Travis failed on some tests in node7 suddenly, when all i did was updated readme...

ThomasBem · 2017-01-26T14:46:16Z

Looks better now? I used IntelliJ instead of VSCode to edit the markdown file.

ThomasBem · 2017-01-26T15:07:56Z

Something going on with Travis? First time i edited the readme.md file, travis failed on node 7.

Now I fixed a small indentation mistake and travis fails on node 4? Even different tests failing.

ThomasBem · 2017-01-27T05:55:03Z

Seems like things are working much better today. So probably some hiccup with Travis yesterday.

sindresorhus · 2017-01-29T08:22:38Z

test/reporters/mini.js

@@ -671,3 +671,26 @@ test('results when hasExclusive is enabled, but there are multiple remaining tes
 	t.end();
 });

+test('Result when no-color flag is set', function (t) {


Result => result

sindresorhus · 2017-01-29T08:22:42Z

test/reporters/verbose.js

@@ -571,6 +571,31 @@ test('results when hasExclusive is enabled, but there are multiple remaining tes
 	t.end();
 });

+test('Result when no-color flag is set', function (t) {


Result => result

sindresorhus · 2017-01-29T08:29:58Z

lib/reporters/mini.js

 	colors[key].enabled = true;
-});
+}


Why are we setting this both here and in the constructor? Wouldn't it make more sense to only set it in the constructor?

Something like this:

chalk.enabled = this.options.color; for (const key of Object.keys(colors)) { colors[key].enabled = this.options.color; }

sindresorhus · 2017-01-29T08:30:06Z

lib/reporters/mini.js

@@ -28,6 +28,13 @@ function MiniReporter(options) {

 	this.options = Object.assign({}, options);

+	if (this.options.color === false) {
+		chalk.enabled = false;
+		for (let key of Object.keys(colors)) {


let => const

sindresorhus · 2017-01-29T08:36:15Z

lib/reporters/mini.js

 	colors[key].enabled = true;
-});
+}


@Qix- This is needed because we assign shortcuts to the colors in lib/colors.js (context #1104 (comment)) Any way we could make the above loop not nessecary? Maybe if we made .enabled property on the prototype?

You mean adding a MiniReporter.prototype.enabled = function () {} that would enable all the colors?

@ThomasBem Ignore this ;) Unrelated to this PR, but rather to chalk.

Ah, oki :) I will commit with updates based on your review, but without doing anything regarding the loop.

I did a quick try where I changed how we create mini and verbose reporters in their test setup. Making sure to include {colors: true} as an options. That way we can use your suggestion:

chalk.enabled = this.options.color; for (const key of Object.keys(colors)) { colors[key].enabled = this.options.color; }

It works for the mini and verbose tests themselves, but it caused this test to fail because chalk and colors did not seem to work as intended, https://github.com/avajs/ava/blob/master/test/cli.js#L73

but it caused this test to fail because chalk and colors did not seem to work as intended, https://github.com/avajs/ava/blob/master/test/cli.js#L73

I think that's as expected as we used to force Chalk in child processes and we no longer do. I guess we can just remove the colors.error() call from that test.

I'm a little lost, could you explain a bit more?

Nvm, Sindre explained it to me. See chalk/chalk#142.

sindresorhus · 2017-01-29T12:58:55Z

@ThomasBem When you push new changes, can you push them as additional commits instead of squashing? Makes it easier to see what changed. We squash on merge anyways.

ThomasBem · 2017-01-29T13:31:12Z

Sure, I can do that :)

Changes this time was all your small suggestions, let => const, Result => result.

novemberborn · 2017-01-29T17:40:53Z

lib/fork.js

@@ -36,7 +36,9 @@ module.exports = (file, opts, execArgv) => {
 		} : false
 	}, opts);

-	const ps = childProcess.fork(path.join(__dirname, 'test-worker.js'), [JSON.stringify(opts)], {
+	var args = [JSON.stringify(opts), opts.color ? '--color' : '--no-color'];


Please use const.

(@sindresorhus is this something xo should enforce?)

I'll add the esnext: true option, which enforces const, when #1154 lands (hopefully in the next few days), as it's the last piece of non-ES2015ified code.

Sorry, C# habbit :p

novemberborn · 2017-01-29T17:44:21Z

lib/babel-config.js

 		message += ' Unexpected Babel configuration for AVA. ';
-		message += 'See ' + chalk.underline('https://github.com/avajs/ava#es2015-support') + ' for allowed values.';
+		message += 'See https://github.com/avajs/ava#es2015-support for allowed values.';


Why this change? (I must admit I didn't read all of the back and forth earlier though… forgive me if I overlooked the answer)

See: #1198 (comment)

@sindresorhus you suggested to remove the color comparison from the test, but this removes it from the actual AVA code.

Why do the changes in this PR require colors to be removed elsewhere? Should we then also remove colors from https://github.com/avajs/ava/blob/master/lib/cli.js#L100:L108?

I removed it from the actual output, since we removed it from test. But if the problem only exits when running tests, not real Ava. Then we can put it back in the implementation.

I did some testing and it seems like this is only a problem in test? Is this because execCli spawns a new process and the chalk.enabled and colors.enabled stuff is not passed on?

As far as I can tell none of the changes I did changed anything related to that behaviour? So why this did work before?

Since it seems to actually work when running Ava properly, we can always just remove the color from expected in the test? At least we catch that the error is thrown with the correct text. Even though colors and underline will be missing.

Oh, I didn't notice this removes it from the actual code. In #1198 (comment), I was talking about removing the chalk call from the test, not elsewhere.

Is this because execCli spawns a new process and the chalk.enabled and colors.enabled stuff is not passed on?

Not exactly. Colors used to be forced "on", now it's auto-detected and colors are automatically off when spawned.

novemberborn · 2017-01-29T17:45:56Z

lib/reporters/verbose.js

+		return new VerboseReporter(options);
+	}
+
+	this.options = Object.assign({}, options);


Assigning this.options seems unnecessary since the constructor didn't receive options before. Nothing else is using this.options.

Qix- · 2017-01-30T21:31:09Z

^{The github mobile site doesn't allow inline comments, so apologies for this being out of line.}

For the child workers, why don't we just utilize support-color's new FORCE_COLOR environment variable instead of pushing down arbitrary flags?

sindresorhus · 2017-01-31T08:07:32Z

@Qix- FORCE_COLOR is for users that can't pass flags. We control the child process, so we can pass flags. I don't really see the problem with that or any benefit in using FORCE_COLOR?

Qix- · 2017-01-31T12:48:31Z

The parent AVA process is passed --colors, etc. Then it sets the environment variable FORCE_COLOR for child processes to force them to match whatever color level the main (parent) process is running at.

No need to do any messy flag juggling or whatever. No need to determine which flags to pass down. No need to update flags in AVA in the future if supports-color changes how it handles flags. This is the perfect use case.

Also, this is exactly what the environment was made for. 🕺

novemberborn · 2017-02-01T14:13:56Z

@Qix- I think your explanation makes sense, but I'll defer to @sindresorhus who I'm sure has got a better understanding of this area than me.

sindresorhus · 2017-02-01T14:56:14Z

The parent AVA process is passed --colors, etc. Then it sets the environment variable FORCE_COLOR for child processes to force them to match whatever color level the main (parent) process is running at.

I still don't see how that's any easier. It's the same amount of work to pass down --color/--no-color based on those flags being set for AVA, as it's passing FORCE_COLOR=0/FORCE_COLOR=1 based on AVA receiving --color/--no-color. The benefit of passing the flags is that we still respect FORCE_COLOR being passed to AVA by the user.

No need to update flags in AVA in the future if supports-color changes how it handles flags. This is the perfect use case.

I don't do what-ifs. We work with how it works now. In the future, the environment variable might be renamed, who knows.

novemberborn · 2017-02-01T16:54:45Z

The benefit of passing the flags is that we still respect FORCE_COLOR being passed to AVA by the user.

👍

…k and colors when throwing bable-config error

… cli.js test and pass the --color flag for the babel config test

ThomasBem · 2017-02-02T20:52:18Z

So I have reverted back to babel-config.js change I made, and instead passed down an additional --color flag in execCli where needed.

Tok me quite a while to get my work in after @vadimdemedes big Magic Assert PR. But I think I managed to get everything up-to-date. But please take a look :)

sindresorhus · 2017-02-08T07:53:48Z

Great work @ThomasBem! Another high-quality pull request :)

novemberborn reviewed Jan 19, 2017

View reviewed changes

ThomasBem force-pushed the no-color-flag branch from 7c00c13 to a59be59 Compare January 24, 2017 18:41

novemberborn mentioned this pull request Jan 25, 2017

Separate config keys from CLI flags #1046

Closed

ThomasBem force-pushed the no-color-flag branch from a59be59 to cdc57cd Compare January 25, 2017 19:23

forresst reviewed Jan 26, 2017

View reviewed changes

ThomasBem force-pushed the no-color-flag branch from cdc57cd to e7183d7 Compare January 26, 2017 14:14

forresst reviewed Jan 26, 2017

View reviewed changes

ThomasBem force-pushed the no-color-flag branch from e7183d7 to 3117c4a Compare January 26, 2017 14:45

ThomasBem force-pushed the no-color-flag branch 2 times, most recently from a30f2dd to 12c8d9e Compare January 27, 2017 05:47

ThomasBem force-pushed the no-color-flag branch from 12c8d9e to f54a35a Compare January 27, 2017 05:57

sindresorhus requested changes Jan 29, 2017

View reviewed changes

ThomasBem force-pushed the no-color-flag branch from f54a35a to 2780f99 Compare January 29, 2017 12:22

novemberborn requested changes Jan 29, 2017

View reviewed changes

novemberborn changed the title ~~[WIP] Added support for --no-color flag in verbose and mini reporters~~ Added support for --no-color flag in verbose and mini reporters Jan 29, 2017

Thomas added 3 commits February 2, 2017 21:10

Added support for --no-color flag in verbose and mini reporters

931501a

Improve use of for...of loop to enable / disable colors. Removed chal…

8f07c41

…k and colors when throwing bable-config error

Reverted changes done to babel-config.js. Enabled chalk and colors in…

ba6b4e9

… cli.js test and pass the --color flag for the babel config test

ThomasBem force-pushed the no-color-flag branch from 12c50c7 to ba6b4e9 Compare February 2, 2017 20:41

novemberborn approved these changes Feb 3, 2017

View reviewed changes

Update cli.js

241073c

sindresorhus approved these changes Feb 8, 2017

View reviewed changes

sindresorhus merged commit 5c4c270 into avajs:master Feb 8, 2017

sindresorhus added a commit that referenced this pull request Feb 8, 2017

Fix some minor issues with #1198

2dc4753

renovate-bot mentioned this pull request Feb 18, 2017

Update dependency ava to version 0.18.2 singapore/gl-got#13

Merged

kevva mentioned this pull request Jun 1, 2017

Remove custom CLI flags in child worker before requiring test file #1393

Closed

Added support for --no-color flag in verbose and mini reporters #1198

Added support for --no-color flag in verbose and mini reporters #1198

Conversation

ThomasBem commented Jan 18, 2017

novemberborn commented Jan 19, 2017

Choose a reason for hiding this comment

sindresorhus Jan 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThomasBem commented Jan 19, 2017

novemberborn commented Jan 20, 2017

sindresorhus commented Jan 23, 2017 • edited Loading

novemberborn commented Jan 23, 2017

sindresorhus commented Jan 23, 2017

ThomasBem commented Jan 24, 2017

novemberborn commented Jan 25, 2017

ThomasBem commented Jan 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThomasBem commented Jan 26, 2017

ThomasBem commented Jan 26, 2017

ThomasBem commented Jan 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented Jan 29, 2017

ThomasBem commented Jan 29, 2017 • edited Loading

Choose a reason for hiding this comment

sindresorhus Jan 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Qix- commented Jan 30, 2017 • edited Loading

sindresorhus commented Jan 31, 2017

Qix- commented Jan 31, 2017 • edited Loading

novemberborn commented Feb 1, 2017

sindresorhus commented Feb 1, 2017 • edited Loading

novemberborn commented Feb 1, 2017

ThomasBem commented Feb 2, 2017

sindresorhus commented Feb 8, 2017

sindresorhus Jan 19, 2017 •

edited

Loading

sindresorhus commented Jan 23, 2017 •

edited

Loading

ThomasBem commented Jan 29, 2017 •

edited

Loading

sindresorhus Jan 29, 2017 •

edited

Loading

Qix- commented Jan 30, 2017 •

edited

Loading

Qix- commented Jan 31, 2017 •

edited

Loading

sindresorhus commented Feb 1, 2017 •

edited

Loading