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.

Sign up for GitHub

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

Jump to bottom

repl: strange regression in output colors #17086

Closed
vsemozhetbyt opened this issue Nov 16, 2017 · 7 comments
Closed

repl: strange regression in output colors #17086

vsemozhetbyt opened this issue Nov 16, 2017 · 7 comments
Labels
confirmed-bug Issues with confirmed bugs. regression Issues related to regressions. repl Issues and PRs related to the REPL subsystem.

Comments

@vsemozhetbyt
Copy link
Contributor

  • Version: 8x-9x
  • Platform: Windows 7 x64
  • Subsystem: util

Compare the outputs of various versions in cmd.exe, pay attention to the string value color:

04 8

06 12

08 9

09 2

10 0
@vsemozhetbyt vsemozhetbyt added the util Issues and PRs related to the built-in util module. label Nov 16, 2017
@addaleax addaleax added repl Issues and PRs related to the REPL subsystem. windows Issues and PRs related to the Windows platform. and removed util Issues and PRs related to the built-in util module. labels Nov 16, 2017
@addaleax
Copy link
Member

@nodejs/platform-windows

@vsemozhetbyt vsemozhetbyt changed the title util: strange regression in output colors repl: strange regression in output colors Nov 16, 2017
@seishun
Copy link
Contributor

seishun commented Nov 17, 2017 •
edited
Loading

The regression was originally introduced in 4fb27d4. (PR: #9266) cc @srl295

@seishun
Copy link
Contributor

seishun commented Nov 17, 2017

And it was fixed in 90a4390 (PR: #16485), which is semver-major. Not sure how. cc @bnoordhuis

@lance
Copy link
Member

lance commented Nov 20, 2017

Per @seishun notes of the commits, perhaps { colors: true } here is responsible for the fix. And though I don't quite understand why this would change the output formatting, the process.versions property was modified here.

@vsemozhetbyt
Copy link
Contributor Author

FWIW, the output in v10 (nightly and canary) is colorless as well.

@seishun
Copy link
Contributor

seishun commented Dec 24, 2017

The regression was introduced again in 727339e (PR: #17565). cc @bnoordhuis

@maclover7 maclover7 added confirmed-bug Issues with confirmed bugs. regression Issues related to regressions. labels Dec 24, 2017
@bnoordhuis bnoordhuis removed the windows Issues and PRs related to the Windows platform. label Dec 25, 2017
@bnoordhuis
Copy link
Member

No, the bug that hid the regression was fixed again. :-)

I don't quite understand why this would change the output formatting, the process.versions property was modified here.

Close, it's the custom formatter on this line, which is unnecessary. I'll open a PR.

bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Dec 25, 2017
@bnoordhuis
repl: fix coloring of process.versions
2a46983 
Remove the custom formatter that was added in commit 4fb27d4 ("intl: Add
more versions from ICU").  It's not necessary anymore (and may not have
been necessary at all) and prevents proper coloring in the REPL.

Fixes: nodejs#17086
@bnoordhuis bnoordhuis mentioned this issue Dec 25, 2017
Closed
MylesBorins pushed a commit that referenced this issue Jan 8, 2018
@bnoordhuis @MylesBorins
repl: fix coloring of process.versions
4c96e96 
Remove the custom formatter that was added in commit 4fb27d4 ("intl: Add
more versions from ICU").  It's not necessary anymore (and may not have
been necessary at all) and prevents proper coloring in the REPL.

PR-URL: #17861
Fixes: #17086
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this issue Jan 9, 2018
@bnoordhuis @MylesBorins
repl: fix coloring of process.versions
566570f 
Remove the custom formatter that was added in commit 4fb27d4 ("intl: Add
more versions from ICU").  It's not necessary anymore (and may not have
been necessary at all) and prevents proper coloring in the REPL.

PR-URL: #17861
Fixes: #17086
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this issue Jan 9, 2018
@bnoordhuis @MylesBorins
repl: fix coloring of process.versions
2cc5053 
Remove the custom formatter that was added in commit 4fb27d4 ("intl: Add
more versions from ICU").  It's not necessary anymore (and may not have
been necessary at all) and prevents proper coloring in the REPL.

PR-URL: #17861
Fixes: #17086
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
gibfahn pushed a commit that referenced this issue Jan 24, 2018
@bnoordhuis @gibfahn
repl: fix coloring of process.versions
bb5cafe 
Remove the custom formatter that was added in commit 4fb27d4 ("intl: Add
more versions from ICU").  It's not necessary anymore (and may not have
been necessary at all) and prevents proper coloring in the REPL.

PR-URL: #17861
Fixes: #17086
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
confirmed-bug Issues with confirmed bugs. regression Issues related to regressions. repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@lance @bnoordhuis @addaleax @seishun @maclover7 @vsemozhetbyt