add optional colors: boolean flag to Options
#21
Conversation
github-actions
bot
added
👋 phase/new
rwe
changed the title
colors: boolean flagcolors: boolean flag to Options
|
Rebased to resolve the trivial import-name conflict. |
There was a problem hiding this comment.
Thanks @rwe!
Appreciate you taking the time to contribute.
@wooorm would probably be the most insightful for reviewing this.
But is both traveling and is in the midst of releasing major versions across most of the 500+ unified ecosystem packages.
Appreciate your patience with the review.
If this is urgent, feel free to ping.
|
Thanks! It's not urgent, just some tinkering I felt might be useful to contribute. For a little context where I was going with this before getting distracted-from-distractions, was to add a "depth" parameter and then use |
|
Thanks for your patience!
If we’re getting more options, sure.
Oh nice! Hmm, I also feel the plural This needs docs! |
| * @property {(_: string) => string} green | ||
| * Style a non-tree value. |
There was a problem hiding this comment.
Can you type the function explicitly somewhere? So @callback StyleFunction ..., and use that in each case here? Then we can describe parameters/return types
| * Configuration. | ||
| * @returns {string} | ||
| * Pretty printed `tree`. | ||
| */ | ||
| export function inspectNoColor(tree, options) { | ||
| return inspectColor(tree, options).replace(colorExpression, '') | ||
| /* c8 ignore next 3 */ |
There was a problem hiding this comment.
Why is this ignored? Wouldn’t it be better to add a test?
| return inspectColor(tree, options).replace(colorExpression, '') | ||
| /* c8 ignore next 3 */ | ||
| const optionsWithNoColor = useColorByDefault | ||
| ? Object.assign({}, options, {colors: false}) |
There was a problem hiding this comment.
Using spreads instead of object.assign is fine by me!
| * @return string | ||
| * The first `count` lines of `text`. | ||
| */ | ||
| const lines = (text, count) => text.split('\n').slice(0, count).join('\n') |
There was a problem hiding this comment.
Describe the parameters, use a regular function, and move it to the bottom of the file. (and: use the braces for the return type, {string})
| @@ -420,6 +430,51 @@ test('inspect()', async function (t) { | |||
| ].join('\n') | |||
| ) | |||
| }) | |||
|
|
|||
| await t.test('inspect(…, {colors: false})', async function () { | |||
There was a problem hiding this comment.
| await t.test('inspect(…, {colors: false})', async function () { | |
| await t.test('should support `colors: false`', async function () { |
| }) | ||
|
|
||
| await t.test( | ||
| 'inspect(…, {colors?: true | null | undefined})', |
There was a problem hiding this comment.
| 'inspect(…, {colors?: true | null | undefined})', | |
| 'should support `colors: true` (default)', |
This comment has been minimized.
This comment has been minimized.
Initial checklist
Description of changes
Previously,
inspect()would always add ANSI color sequences when on Node and always omit them otherwise.inspectNoColor()andinspectColor()were exported to override this, but that was a little bit clunky. This adds an optionalcolors: booleanflag toOptions. (Note that this is the same flag name as innode:util'sinspect).The default behaviour is unchanged: this feature is purely additive.
Internally, this also introduces the colorization/stylization functions to
State, allowing the no-color path to simply omit colorization, rather than stripping ANSI sequences after the fact.