Skip to content

util: add colorText method - #43371

Closed
hemanth wants to merge 7 commits into
nodejs:mainfrom
hemanth:util-format-text
Closed

util: add colorText method#43371
hemanth wants to merge 7 commits into
nodejs:mainfrom
hemanth:util-format-text

Conversation

@hemanth

@hemanthhemanth commented Jun 10, 2022

Copy link
Copy Markdown
Contributor

This is with reference to the comment in #42770

A few things to finalize before completing this draft PR:

  • Are we Ok to have such a method?
  • Given that is it CLI only the name sounds fine?
  • Can it be part of the console API?

//cc @nodejs/util

@nodejs-github-botnodejs-github-bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Jun 10, 2022
Comment threadtest/parallel/test-util-colorText.js Outdated
Comment threadlib/util.js Outdated
@ljharb

Copy link
Copy Markdown
Member

Name sounds good to me; i don't think it should be in console at all.

Is there any way to have tests that actually verify that the expected colors show up on supported platforms?

@cjihrigcjihrig left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Left some preliminary comments.

Is there any way to have tests that actually verify that the expected colors show up on supported platforms?

test/parallel/test-util-format.js and test/parallel/test-util-inspect.js might be good places to look for examples.

Also cc @BridgeAR who might be interested since this overlaps with the color support in util.inspect().

Comment threadlib/util.js Outdated
Comment threadlib/util.js Outdated
Comment threadlib/util.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If this is going to be public API, we should probably provide some additional validation for supported formats.

@hemanthhemanthJun 11, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

additional validation

like?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was thinking we could validate that format is actually something supported by inspect.colors, but feel free to ignore if you want.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

If they pass anything apart as of now, we are just returning the text as in, do you feel it makes sense to check if the format is supported by inspect.colors and throw an error for non-supported inputs?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, that's what I originally meant.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is inspect.colors mutable and exposed to users?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

^ yes!

Comment threadlib/util.js Outdated
Comment threadtest/parallel/test-util-colorText.js Outdated
Comment threadtest/parallel/test-util-colorText.js Outdated
@hemanthhemanth changed the title WIP: Adding colorText method to util.util: [WIP] Adding colorText method to util.Jun 11, 2022
@hemanth
hemanthforce-pushed the util-format-text branch 3 times, most recently from 9c94ffe to 9ac1a84CompareJune 12, 2022 06:03
@hemanthhemanth changed the title util: [WIP] Adding colorText method to util.util: Adding colorText method to utilJun 13, 2022
@Trott

Copy link
Copy Markdown
Member

Micro-nit on the commit message: The verb in the first line of the commit message should be imperative. In other words, add rather than adding. This is something that someone can fix when landing, and also no one (not even me) is actually going to care if it slips through as adding rather than add. But if you want to make that change, that would make it comply with our contributor guidelines doc.

@hemanth

Copy link
Copy Markdown
ContributorAuthor

Thanks for the clarification @Trott. I had that doubt, fixed it.

@TrottTrott added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 14, 2022
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 14, 2022
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@hemanth

Copy link
Copy Markdown
ContributorAuthor

We should also add this to the docs right?

@cjihrig

Copy link
Copy Markdown
Contributor

Yes, this would require a docs update.

@hemanth

Copy link
Copy Markdown
ContributorAuthor

Is this ready enough?

@hemanthhemanth changed the title util: Adding colorText method to utilutil: add colorText methodJun 20, 2022
@hemanth
hemanth requested a review from cjihrigJune 20, 2022 15:48
@hemanth
hemanth requested a review from aduh95June 20, 2022 15:48

@cjihrigcjihrig left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Left a few comments. It would be good for others to weigh in on things like the overall API design and whether or not we should make this experimental first.

Comment threaddoc/api/util.md Outdated
Comment threaddoc/api/util.md Outdated
added: v18.3.0
-->

* `format` {string} `format` one of the color format from `util.inspect.colors`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should probably link util.inspect.colors to https://nodejs.org/api/util.html#customizing-utilinspect-colors.

Comment threaddoc/api/util.md Outdated
Comment threaddoc/api/util.md Outdated
Comment threaddoc/api/util.md Outdated
Comment threaddoc/api/util.md
Takes `format` and `text` and retuns the colored text form

```js
const util = require('node:util');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you remove this line. Otherwise we'll need to show the same example code for CJS and ESM.

Comment threaddoc/api/util.md
const util = require('node:util');

console.log(util.colorText('red', 'This text shall be in red color'));
// ^ '\u001b[31mThis text shall be in red color\u001b[39m'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
// ^ '\u001b[31mThis text shall be in red color\u001b[39m'
// ^ '\u001b[31mThis text shall be colored red\u001b[39m'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

s/shall be/is?

Comment threadlib/util.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, that's what I originally meant.

Comment on lines +26 to +31
assert.throws(() => {
util.colorText('red', undefined);
}, {
code: 'ERR_INVALID_ARG_TYPE',
message: 'The "text" argument must be of type string. Received undefined'
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wouldn't this already be tested on line 20 when invalidOption is undefined?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Right, this should rather be util.colorText(undefined, undefined);

Comment threaddoc/api/util.md Outdated
Comment threaddoc/api/util.md
const util = require('node:util');

console.log(util.colorText('red', 'This text shall be in red color'));
// ^ '\u001b[31mThis text shall be in red color\u001b[39m'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

s/shall be/is?

Comment threadlib/util.js Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is inspect.colors mutable and exposed to users?

hemanthand others added 5 commits June 20, 2022 21:52
Co-authored-by: Colin Ihrig <cjihrig@gmail.com>
Co-authored-by: Colin Ihrig <cjihrig@gmail.com>
Co-authored-by: Colin Ihrig <cjihrig@gmail.com>
Co-authored-by: Colin Ihrig <cjihrig@gmail.com>
Co-authored-by: Jordan Harband <ljharb@gmail.com>

@BridgeARBridgeAR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am hesitant to add new formatting functionalities to util (console does not seem ideal either). I just opened a something similar API: #43523. It does a bit more by also allowing nested colors, this functionality here does not do that (e.g., colorText('green', 'green ' + colorText('red', 'red') + ' green') would end up being colored: green red defaultColor instead of green red green).

@jasnell

Copy link
Copy Markdown
Member

Let's throw this into the TSC agenda as part of the discussion for #43523

@jasnelljasnell added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Jun 27, 2022
@mcollinamcollina removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Jul 13, 2022
@mcollina

Copy link
Copy Markdown
Member

Dropping it from the tsc-agenda until #43382 is resolved.

@mcollinamcollina added the blocked PRs that are blocked by other issues or PRs. label Jul 13, 2022
Comment threaddoc/api/util.md
added: REPLACEME
-->

* `format` {string} A color format defined in `util.inspect.colors`.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Accepting string | string[] would be really helpful to e.g. make text both a certain color and underline/italic/bold/... at the same time

@mcollina

Copy link
Copy Markdown
Member

Superseded by #51850

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

blockedPRs that are blocked by other issues or PRs.needs-ciPRs that need a full CI run.utilIssues and PRs related to the built-in util module.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants

@hemanth@ljharb@Trott@nodejs-github-bot@cjihrig@jasnell@mcollina@BridgeAR@felixfbecker@aduh95