Conversation
| func makeColorFunc(color string) func(string) string { | ||
| return func(arg string) string { | ||
| output := arg | ||
| if isatty.IsTerminal(os.Stdout.Fd()) { |
There was a problem hiding this comment.
This works for stdout, but what if we wanted to output color to stderr in case it's a tty, separately from stdout which might be redirected elsewhere?
I'm wondering whether it would be useful to have an object that could be initialized with an output stream and has color functions available on it that are all no-op unless the stream is a tty:
// within a Cobra command
out := cmd.OutOrStdout()
ansiStdout := utils.NewAnsi(out)
ansiStderr := utils.NewAnsi(cmd.OutOrStderr())
fmt.Fprintln(out, ansiStdout.Cyan("hello on stdout"))
cmd.Println(ansiStderr.Cyan("some error"))Or maybe we can just cross that bridge when we get there?
There was a problem hiding this comment.
I've been studying Stripe CLI and this is how they handle it https://github.com/stripe/stripe-cli/blob/a8a3108b998c56bd29e0c4e1763920163de59f8d/pkg/ansi/ansi.go#L162-L172
There was a problem hiding this comment.
I like how clear the interface @vilmibm came up with to handle the color, the complexity of showing or not showing color his hidden away (in a good way!). Maybe I'm underestimating how often stdout and stdin's tty status differs, but I feel like keeping the interface simple is more important at this point than having it work correctly in all scenarios.
There was a problem hiding this comment.
Sure, that's a good point @probablycorey. We don't have to worry about stderr for now until we need to print ansi codes to stderr too.
For clarity, these are some cases when stdout and stderr tty status differ:
gh pr list | pipe # stdout is to a pipe, stderr is tty
gh pr list 2>error.log # stdout is tty, stderr is file
| // This is really annoying. If you just define Bold as ColorFunc("+b") it will properly bold but | ||
| // will not use the default color, resulting in black and probably unreadable text. This forces | ||
| // the default color before bolding. | ||
| output = ansi.Color(ansi.DefaultFG+arg+ansi.Reset, "+b") |
There was a problem hiding this comment.
Bummer, but good catch!
| func makeColorFunc(color string) func(string) string { | ||
| return func(arg string) string { | ||
| output := arg | ||
| if isatty.IsTerminal(os.Stdout.Fd()) { |
There was a problem hiding this comment.
Sure, that's a good point @probablycorey. We don't have to worry about stderr for now until we need to print ansi codes to stderr too.
For clarity, these are some cases when stdout and stderr tty status differ:
gh pr list | pipe # stdout is to a pipe, stderr is tty
gh pr list 2>error.log # stdout is tty, stderr is file
|
Going to merge this as-is since it fixes currently broken behavior. i'm totally into more nuanced support for the stderr/stdout divergence case @mislav brought up in the future -- we should have an issue for it 😉 |
Fixed parseFileArgs, reformatted tests
3 participants
This PR disables all escape codes when printing via
utilsifghdetects we're not attached to a terminal.