util: detecting terminal capabilities in styleText #51959
Conversation
nodejs-github-bot
added
needs-ci
github-actions
bot
removed
the
request-ci
| @@ -204,6 +204,12 @@ function pad(n) { | |||
| * @returns {string} | |||
| */ | |||
| function styleText(format, text) { | |||
| const env = process.env; | |||
| if (!process.stdout.isTTY || | |||
There was a problem hiding this comment.
please use shouldColorize from internal/util/colors, it takes all these into account but also the actual color depth of the tty.
There was a problem hiding this comment.
I will follow this suggestion based on the feedback for my proposal below
| @@ -204,6 +204,12 @@ function pad(n) { | |||
| * @returns {string} | |||
| */ | |||
| function styleText(format, text) { | |||
There was a problem hiding this comment.
if we add this we probably want to pass a force parameter (not an env var)
| @@ -204,6 +204,12 @@ function pad(n) { | |||
| * @returns {string} | |||
| */ | |||
| function styleText(format, text) { | |||
| const env = process.env; | |||
| if (!process.stdout.isTTY || | |||
There was a problem hiding this comment.
I'm not sure it's the right thing to systematically look at process.stdout. styleText, as an utility function, should not (always) depend on that.
There was a problem hiding this comment.
Do you mean the user should perform this check by themselves?
There was a problem hiding this comment.
When using such a utility, you aren't necessarily writing on to stdout. You might be writing to a file, and you might want to use coloring even without a tty
There was a problem hiding this comment.
What about adding a boolean parameter to the function set to true by default to check the TTY: styleText(format, text, validateTTY)? So we offer the option to use the function with or without the proposed validation?
The idea behind this validation is to provide the mechanism by default to Node users, saving them time with some code that would be a must in the most common use case of this utility function.
I will be waiting for your feedback, @targos and @MoLow to implement such suggestion
|
This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open. |
|
@edsadr are you still planning to persue this? |
|
I will work on that. Possibly in a different PR. |
|
Closing in favour of #54389 |
This PR adds some detection of the process configuration for printing colors when using
util.styleText