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

Revert "util: use blue on non-windows systems for number/bigint" #19256

Closed
wants to merge 1 commit into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Mar 9, 2018

This reverts commit 1708af3.

Numbers are much more difficult to read in blue and it would be good
to have a consistent output throughout all OS.

Refs: #18925

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

This reverts commit 1708af3.

Numbers are much more difficult to read in blue and it would be good
to have a consistent output throughout all OS.
@BridgeAR BridgeAR requested review from devsnek and a team March 9, 2018 13:45
@nodejs-github-bot nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Mar 9, 2018
@BridgeAR
Copy link
Member Author

BridgeAR commented Mar 9, 2018

@devsnek
Copy link
Member

devsnek commented Mar 9, 2018

i understand consistent output but Numbers are much more difficult to read in blue is objective based on your stylistic choices for your terminal. i won't block this but i am opposed to it.

@BridgeAR
Copy link
Member Author

BridgeAR commented Mar 9, 2018

@devsnek that is correct but the same applies to using blue. I personally would like to have a consistent output throughout all OS as well and I think changing it was a mistake.

@BridgeAR BridgeAR requested a review from a team March 9, 2018 14:28
@BridgeAR BridgeAR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Mar 9, 2018
@gibfahn
Copy link
Member

gibfahn commented Mar 9, 2018

that is correct but the same applies to using blue.

AIUI the original colour was blue, then it was changed everywhere to make it easier to read on Windows. In my experience yellow on a white background is about as hard to read as blue on a black background, and differentiating numbers from other things is definitely useful.

I personally would like to have a consistent output throughout all OS as well

Why?

Fundamentally we're not going to find something that works perfectly in all terminals, so I think the current master is a good compromise.

So I'd be -1 on this PR.

EDIT: If I'm the only one disagreeing then I won't stand in the way of this PR, so -0.

@BridgeAR
Copy link
Member Author

BridgeAR commented Mar 9, 2018

Numbers were yellow for six years now...

I wonder if changing it to blue should not have been a BC actually. Likely a lot of people will have to switch their setting now. There is no "right" or "wrong" since in each case people will be unhappy but I would like to stick to the way it was instead of breaking the old behavior.

Copy link
Member

@ChALkeR ChALkeR left a comment

Choose a reason for hiding this comment

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

Tested on all pre-bundled color schemes of KDE terminal — Konsole.

Yellow looks good on all 12 of them (including ones with white bg).

Blue looks unreadable on 3 (25%) of them, including the probably widely used «White on black» colorscheme.

Given that I don't remember significant complaints about the previous colorscheme over long time, I suggest to revert to it.

@ChALkeR ChALkeR requested review from a team, addaleax and lpinca March 9, 2018 17:16
@lpinca
Copy link
Member

lpinca commented Mar 9, 2018

I have no strong opinion, I'm fine with both colors so I won't approve nor block this.

@ChALkeR
Copy link
Member

ChALkeR commented Mar 9, 2018

Also, in standard Linux virtual console blue looks much less readable than yellow.
To test: printf "\x1B[34m001\n\x1B[0m"; printf "\x1B[33m001\n\x1B[0m";

@addaleax
Copy link
Member

@nodejs/tsc If there are no objections, I’d like to land this soon – it’s a revert and I think we shouldn’t be shy about that.

I do agree that blue looks nice, but even in that case we should probably stick to the current default, and maybe provide some way to configure things via an env var or so if we want to. ¯\_(ツ)_/¯

@gibfahn
Copy link
Member

gibfahn commented Mar 15, 2018

If there are no objections, I’d like to land this soon – it’s a revert and I think we shouldn’t be shy about that.

SGTM

@addaleax
Copy link
Member

Landed in 1329844

@addaleax addaleax closed this Mar 17, 2018
addaleax pushed a commit that referenced this pull request Mar 17, 2018
This reverts commit 1708af3.

Numbers are much more difficult to read in blue and it would be good
to have a consistent output throughout all OS.

PR-URL: #19256
Refs: #18925
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@BridgeAR BridgeAR deleted the revert-18925 branch April 1, 2019 23:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.