chore: clean up color/tty handling core in cli-repl #387
Conversation
| getFormatOptions(): { colors: boolean } { | ||
| return { | ||
| colors: this.repl ? this.repl.useColors : | ||
| process.stdout.isTTY && process.stdout.getColorDepth() > 1 |
packages/cli-repl/src/arg-parser.ts
Outdated
| } | ||
| throw new Error( | ||
| ` ${clr(i18n.__(UNKNOWN), ['red', 'bold'])} ${clr(parameter, 'bold')} | ||
| ` ${clr(i18n.__(UNKNOWN), ['red', 'bold'], { colors: true })} ${clr(parameter, 'bold', { colors: true })} |
There was a problem hiding this comment.
why here is ok to set colors= true?
There was a problem hiding this comment.
It’s not really, but we’re also not explicitly printing anything here … if I understand correctly, this gets passed down to the Node.js error handler itself
So a “clean” solution would be e.g. creating an error object here that has a colored and a non-colored message, and then handling that at the parseCliArgs call side in src/run.ts and deciding what to print from there … but I’m not sure if that’s worth the extra complexity? Especially that we’re targeting scripted usage with the color-on/color-off handling, and wouldn’t typically contain argument name misspellings (or not for a long time :))
There was a problem hiding this comment.
Fwiw, I’ve made this use the same thing as above, i.e. basically assume that this ends up on stderr (which I think is accurate)
| terminal: true, | ||
| breakEvalOnSigint: true, | ||
| preview: false, | ||
| terminal: process.env.MONGOSH_FORCE_TERMINAL ? true : undefined |
| }; | ||
|
|
||
| const originalEditorAction = this.repl.commands.editor.action.bind(this.repl); | ||
| if (this.repl.commands.editor) { |
packages/cli-repl/src/clr.ts
Outdated
|
|
||
| export default function clr(text: string, style: any): string { | ||
| return process.stdout.isTTY ? ansi.format(text, style) : text; | ||
| export default function clr(text: string, style: string|string[], options: { colors: boolean }): string { |
There was a problem hiding this comment.
Wouldn't be a bit confusing without context for a clr function (that i believe stands for color) to take a color true | false argument?
Should we rename something, or do something to make this clear?
There was a problem hiding this comment.
I would absolutely be in favor of renaming clr to something more expressive :) I’ll try to think of something
There was a problem hiding this comment.
I’ve renamed the functions themselves but not the places where they were imported, still not ideal but at least not that strongly abbreviated
Don’t assume that `process.stdout.isTTY` is a good indicator for coloring strings: - Not all terminals have color support - We should honour environment variables like `NO_COLOR` - Not all formatted strings end up printed to stdout Also, don’t tell the Node.js repl that we are working with a terminal if we don’t know so for sure.
addaleax
force-pushed
the
cli-repl-color
branch
from
6e43050 to
f969629
Compare
3 participants
Don’t assume that
process.stdout.isTTYis a good indicatorfor coloring strings:
NO_COLORAlso, don’t tell the Node.js repl that we are working with a terminal
if we don’t know so for sure.