Fix picomatch basename option, add NO_COLOR support, bump engine to >…#10
Conversation
…=18.3 - Use picomatch's `basename` option instead of micromatch's `matchBase` - Respect NO_COLOR env var and non-TTY stdout for ANSI color output - Bump minimum Node.js engine to >=18.3.0 (parseArgs availability)
There was a problem hiding this comment.
Pull request overview
Updates glob matching and CLI output behavior to better align with picomatch options and common terminal color expectations, while also raising the minimum supported Node.js version to ensure required runtime APIs are available.
Changes:
- Switch glob matching from micromatch-style
matchBaseto picomatch’sbasenameoption. - Disable ANSI colors when
NO_COLORis set or stdout is non-TTY (withFORCE_COLORoverride). - Bump
engines.nodeminimum to>=18.3.0.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/validator/glob.ts |
Uses picomatch’s basename option for basename-only glob patterns. |
src/output/text.ts |
Adds environment/TTY-based gating for ANSI color output. |
package.json |
Raises minimum Node.js engine version to >=18.3.0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const colorEnabled = | ||
| !('NO_COLOR' in process.env) && | ||
| (process.stdout.isTTY === true || process.env.FORCE_COLOR !== undefined); |
There was a problem hiding this comment.
FORCE_COLOR is treated as an enable flag whenever it is present, which means FORCE_COLOR=0 will still enable ANSI output (and also won’t disable colors on TTY). If you want to support FORCE_COLOR, it should interpret the value and treat 0 as disabling colors (and optionally allow 1+ to force-enable), otherwise redirected output can unexpectedly contain ANSI sequences.
| const colorEnabled = | |
| !('NO_COLOR' in process.env) && | |
| (process.stdout.isTTY === true || process.env.FORCE_COLOR !== undefined); | |
| const noColor = 'NO_COLOR' in process.env; | |
| const forceColorValue = process.env.FORCE_COLOR; | |
| let colorEnabled: boolean; | |
| if (noColor) { | |
| // NO_COLOR always disables colors, regardless of FORCE_COLOR or TTY | |
| colorEnabled = false; | |
| } else if (forceColorValue !== undefined) { | |
| // Interpret FORCE_COLOR according to common conventions: | |
| // - "0" => disable colors | |
| // - "1"+ => force-enable colors | |
| colorEnabled = forceColorValue !== '0'; | |
| } else { | |
| // Default: enable colors only when output is a TTY | |
| colorEnabled = process.stdout.isTTY === true; | |
| } |
| const colorEnabled = | ||
| !('NO_COLOR' in process.env) && | ||
| (process.stdout.isTTY === true || process.env.FORCE_COLOR !== undefined); |
There was a problem hiding this comment.
Color enablement now depends on NO_COLOR, TTY detection, and FORCE_COLOR, but this behavior isn’t covered by tests. Since CLI integration tests exist, please add cases to assert: (1) default output is uncolored when stdout is non-TTY, (2) NO_COLOR disables coloring even when forced, and (3) FORCE_COLOR forces coloring when stdout is non-TTY (including the FORCE_COLOR=0 disabling case if supported).
| const colorEnabled = | |
| !('NO_COLOR' in process.env) && | |
| (process.stdout.isTTY === true || process.env.FORCE_COLOR !== undefined); | |
| const noColor = 'NO_COLOR' in process.env; | |
| const forceColor = process.env.FORCE_COLOR; | |
| const colorEnabled = !noColor && ( | |
| forceColor === '0' | |
| ? false | |
| : forceColor !== undefined | |
| ? true | |
| : process.stdout.isTTY === true | |
| ); |
…=18.3
basenameoption instead of micromatch'smatchBase