util: support array of formats in util.styleText

[marco-ippolito]

Fixes: #52035

[rauschma]

Looking at the code:

if (ArrayIsArray(format) && format.length > 0) {

What happens if the Array is empty? Maybe add a unit test for that.

[marco-ippolito]

Looking at the code:

if (ArrayIsArray(format) && format.length > 0) {

What happens if the Array is empty? Maybe add a unit test for that.

it's already present before:
https://github.com/nodejs/node/blob/59845783dd3681eb005186f0ebd542dcd2757bbe/test/parallel/test-util-styletext.js#L15

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/57677/

[marco-ippolito]

I'm only not sure of the order in which we should add/remove styles:

For:

util.styleText(['red','bold'],'test')

\u001b[31m: color to red.
\u001b[1m: style to bold.
test: The actual text content.
\u001b[39m: resets color.
\u001b[22m: removes bold.

OR

\u001b[1m: style to bold.
\u001b[31m: color to red.
test: The actual text content.
\u001b[39m: resets color.
\u001b[22m: removes bold.

Probably second one is more correct?

[rauschma]

I was asking because of this (I don’t know if that is intended or not):

if(ArrayIsArray(format)&&format.length>0){// ···return`${formatCodes}${text}${append}`;}// ❌ `format` might be the empty Array hereconstformatCodes=inspect.colors[format];if(formatCodes==null){validateOneOf(format,'format',ObjectKeys(inspect.colors));// ···}

[marco-ippolito]

I was asking because of this (I don’t know if that is intended or not):

if(ArrayIsArray(format)&&format.length>0){// ···return`${formatCodes}${text}${append}`;}// ❌ `format` might be the empty Array hereconstformatCodes=inspect.colors[format];if(formatCodes==null){validateOneOf(format,'format',ObjectKeys(inspect.colors));// ···}

If the array is empty it will throw ERR_INVALID_ARG_VALUE, it was already covered before my implementation and test

[rauschma]

Probably second one is more correct?

I’d say the following two invocations should produce the same result:

util.styleText('bold',util.styleText('red','Bold and red!'))util.styleText(['bold','red'],'Bold and red!')

[marco-ippolito]

Probably second one is more correct?

I’d say the following two invocations should produce the same result:

util.styleText('bold',util.styleText('red','Bold and red!')),util.styleText(['bold','red'],'Bold and red!'),

agreed, added a test to verify

[rauschma]

Another option is that an empty Array simply does nothing. Use case:

conststyles=[];if(warning){styles.push('red');}console.log(util.styleText(styles,message));

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/57689/

[IlyasShabi]

LGTM

[rauschma]

Just in case: You can chain multiple codes via ; – which saves you a few characters in the Array case. But that may be too much effort for something that’s less frequently used.

This could look as follows (code may contain bugs – I haven’t tested it and am a bit tired):

/** * @param {number[]} codes * @returns {string} */functionescapeStyleCodes(codes){return`\x1B[`+codes.join(';')+`m`;}/** * @param {string | string[]} format * @param {string} text * @returns {string} */functionstyleText(format,text){validateString(text,'text');if(ArrayIsArray(format)){conststartCodes=[];constendCodes=[];for(constkeyofformat){constformatCodes=inspect.colors[key];if(formatCodes==null){validateOneOf(key,'format',ObjectKeys(inspect.colors));}startCodes.push(formatCodes[0]);endCodes.unshift(formatCodes[1]);}returnescapeStyleCodes(startCodes)+text+escapeStyleCodes(endCodes);}constformatCodes=inspect.colors[format];if(formatCodes==null){validateOneOf(format,'format',ObjectKeys(inspect.colors));}returnescapeStyleCodes([formatCodes[0]])+text+escapeStyleCodes([formatCodes[1]]);}

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/57707/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/57708/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/57709/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/57710/

[nodejs-github-bot]

Landed in 9a1e01c