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

ConsoleReporter: do not color if !isTTY #6336

Merged
merged 4 commits into from
Sep 5, 2018
Merged

ConsoleReporter: do not color if !isTTY #6336

merged 4 commits into from
Sep 5, 2018

Conversation

jrr
Copy link
Contributor

@jrr jrr commented Aug 30, 2018

Summary

Given the general problem of yarn emitting console color codes at inappropriate times, this PR tackles one particular case that's been annoying me.

Before:

jrr@jrrmbp ~> yarn versions |more
yarn versions v1.9.4
{ yarn: ESC[32m'1.9.4'ESC[39m,
  http_parser: ESC[32m'2.8.0'ESC[39m,
  node: ESC[32m'8.11.3'ESC[39m,
  v8: ESC[32m'6.2.414.54'ESC[39m,
  uv: ESC[32m'1.19.1'ESC[39m,
  zlib: ESC[32m'1.2.11'ESC[39m,
  ares: ESC[32m'1.10.1-DEV'ESC[39m,
  modules: ESC[32m'57'ESC[39m,
  nghttp2: ESC[32m'1.32.0'ESC[39m,
  napi: ESC[32m'3'ESC[39m,
  openssl: ESC[32m'1.0.2o'ESC[39m,
  icu: ESC[32m'60.1'ESC[39m,
  unicode: ESC[32m'10.0'ESC[39m,
  cldr: ESC[32m'32.0'ESC[39m,
  tz: ESC[32m'2017c'ESC[39m }
Done in 0.02s.

After:

jrr@jrrmbp ~/r/yarn (fix/console-reporter-colors)> ./bin/yarn versions |more
yarn versions v1.10.0-0
{ yarn: '1.10.0-0',
  http_parser: '2.8.0',
  node: '8.11.3',
  v8: '6.2.414.54',
  uv: '1.19.1',
  zlib: '1.2.11',
  ares: '1.10.1-DEV',
  modules: '57',
  nghttp2: '1.32.0',
  napi: '3',
  openssl: '1.0.2o',
  icu: '60.1',
  unicode: '10.0',
  cldr: '32.0',
  tz: '2017c' }
Done in 0.13s.

See issue here: #728


This band-aids one particular symptom that has been annoying me in my CI output, but perhaps in the future a larger refactor could:

  • reduce the number of special cases (there's a surprising number of isTTY checks in a few different files)
  • rely on Chalk's built-in terminal detection. (shouldn't we get this for free? "Color support is automatically detected", https://github.com/chalk/chalk#chalklevel)

@jrr jrr changed the title strip colors if !isTTY ConsoleReporter: strip colors if !isTTY Aug 30, 2018
@arcanis
Copy link
Member

arcanis commented Sep 3, 2018

While I understand the rational behind this change, I'd prefer if you could simply disable the colors before they are added. Triming them is a tad more generic, but seems like a hack to me:

value = inspect(value, {
breakLength: 0,
colors: true,
depth: null,
maxArrayLength: null,
});

Copy link
Member

@arcanis arcanis left a comment

Choose a reason for hiding this comment

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

Please disable the colors rather than post-process the messages.

@jrr
Copy link
Contributor Author

jrr commented Sep 4, 2018

@arcanis thanks for the feedback, and for pointing me toward the inspect() call. This is my first time inside yarn and I'm pretty lost.

Agree that it's better to not even emit colors in the first place, rather than strip them out afterward. And that brings the diff down to one line!

@jrr jrr changed the title ConsoleReporter: strip colors if !isTTY ConsoleReporter: do not color if !isTTY Sep 4, 2018
Copy link
Member

@arcanis arcanis left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@arcanis arcanis merged commit 83bd87e into yarnpkg:master Sep 5, 2018
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.

2 participants