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

Be smarter about “dumb” terminals #89

Closed
wants to merge 1 commit into from
Closed

Be smarter about “dumb” terminals #89

wants to merge 1 commit into from

Conversation

Alhadis
Copy link
Contributor

@Alhadis Alhadis commented Dec 2, 2018

When terminals are declared as "dumb", it means the author doesn't want/expect anything other than plain-text output. There's usually a damn good reason for this — we should treat TERM=dumb as meaning FORCE_COLOR=0, instead of imagining this.

Fixes: chalk/supports-color#88
Related: mochajs/mocha#1304, emacs.stackexchange.com/q/27218

Emacs compile command — Before and after:

Emacs compilation-mode: Before Emacs compilation-mode: After

@plroebuck As you can see, it seems some changes are still required elsewhere to clean up the stack-trace highlighting.

This fix has the unfortunate effect of impacting Emacs commands which do colourise output directly, such as mocha.el's (mocha-test-project) command:

mocha.el — Before and after:

Mocha.el: Before Mocha.el: After

Authors can always fix this by forcing colours in their environments, though, so this shouldn't be a problem in the long-run.

@Qix-
Copy link
Member

Qix- commented Dec 2, 2018

Yeah this looks fine to me. Could you add a test that would fail under previous scenarios?

@Alhadis
Copy link
Contributor Author

Alhadis commented Dec 2, 2018

What sort of test are we looking at? Did you want me to write a Lisp script to automate Emacs to run a compiled build with mocha, and do it again with the elevated priority of the TERM=dumb variable?

@Qix-
Copy link
Member

Qix- commented Dec 2, 2018

Less about Emacs, more just the fact that TERM_PROGRAM was being honored at a higher priority than TERM=dumb.

@Alhadis
Copy link
Contributor Author

Alhadis commented Dec 2, 2018

Ah good, because using Emacs as a general-purpose language interpreter tends to get bulky and messy quick... 😟

@Alhadis
Copy link
Contributor Author

Alhadis commented Dec 2, 2018

Is this good enough??

test('disables colours entirely if `TERM=dumb` is in env', t => {
	process.stdout.isTTY = true;
	process.stderr.isTTY = true;
	process.env.TERM = 'dumb';
	const result = importFresh('.');
	t.truthy(result.stdout);
	t.truthy(result.stderr);
	t.is(result.stdout.level, 1);
	t.is(result.stdout.level, 1);
});

test('it allows `TERM=dumb` to be overridden by `FORCE_COLOR` in env', t => {
	process.stdout.isTTY = false;
	process.env.TERM = 'dumb';
	process.env.FORCE_COLOR = true;
	const result = importFresh('.');
	t.truthy(result.stdout);
	t.is(result.stdout.level, 1);
});

I figured out what the spurious highlighting was before; some auto-reformatter thing was modifying code while I was saving changes, and I didn't even notice . I'm assuming that's that xo thing? D:

This was the newest error I just choked on. D:

 disables colours entirely if `TERM=dumb` is in env

  /Users/john/Forks/supports-color/test.js:19

   18:   const result = importFresh('.');
   19:   t.truthy(result.stdout);        
   20:   t.truthy(result.stderr);        

  Value is not truthy:

  false

  result.stdout
  => false

Look, I don't know what you're using to run your tests, or why importFresh() exists, or what half of these flags and levels do or mean. But I really don't have the time or patience for this, I'm sorry. Can I leave it to you to add a test for this, please?

@Qix-
Copy link
Member

Qix- commented Dec 3, 2018

One needs to check the TERM_PROGRAM as well.

@plroebuck
Copy link
Contributor

plroebuck commented Dec 3, 2018

I'd fix "tests.js" myself, but don't see "command line instructions" in the merge box to checkout the commit.

Nvm. Made my own...

@Alhadis
Copy link
Contributor Author

Alhadis commented Dec 4, 2018

Closing in favour of #89

@Alhadis Alhadis closed this Dec 4, 2018
@Alhadis Alhadis deleted the dumb branch December 4, 2018 08:46
@Qix-
Copy link
Member

Qix- commented Dec 4, 2018

Psst.. this is #89 ;) think you mean #90 haha

@Alhadis
Copy link
Contributor Author

Alhadis commented Dec 4, 2018

LOL, yes. My bad.

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.

3 participants