fix: check if stderr is a tty #51
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
help.go
Outdated
| fmt.Fprintln(w, err.Error()) | ||
| return | ||
| } |
There was a problem hiding this comment.
This return implies usage errors won't be displayed to user.
There was a problem hiding this comment.
Let me rephrase: it might be OK, but then an inline comment expanding the reason would be present
There was a problem hiding this comment.
yes, it's on purpose, it'll still show the error, just not the try --help thing
There was a problem hiding this comment.
Thanks for tge confirmation
Then an inline comment would be appreciated
There was a problem hiding this comment.
Pull Request Overview
This PR fixes #50 by detecting when stderr is not a terminal (TTY) and falling back to plain error output instead of styled output.
- Imported
github.com/charmbracelet/x/termand updated theErrorHandlerdoc to note terminal-only styling. - Added a TTY check around styled error handling in
Execute. - Prints unstyled errors when stderr is redirected or piped (non-TTY).
Comments suppressed due to low confidence (1)
fang.go:163
- Add unit tests covering both branches (TTY vs. non-TTY) to ensure plain error output is used when stderr is redirected and styled output when running in a terminal.
if !term.IsTerminal(w.Fd()) {
fang.go
Outdated
| if w, ok := root.ErrOrStderr().(term.File); ok { | ||
| // if stderr is not a tty, simply print the error without any | ||
| // styling or going through an [ErrorHandler]: | ||
| if !term.IsTerminal(w.Fd()) { | ||
| fmt.Fprintln(w, err.Error()) | ||
| return err | ||
| } |
There was a problem hiding this comment.
The type assertion to term.File is too strict and may miss other io.Writer implementations wrapping *os.File. Consider asserting an interface with Fd() (e.g., interface{ Fd() uintptr }) or checking for *os.File directly to reliably detect a terminal descriptor.
| if w, ok := root.ErrOrStderr().(term.File); ok { | |
| // if stderr is not a tty, simply print the error without any | |
| // styling or going through an [ErrorHandler]: | |
| if !term.IsTerminal(w.Fd()) { | |
| fmt.Fprintln(w, err.Error()) | |
| return err | |
| } | |
| if w, ok := root.ErrOrStderr().(interface{ Fd() uintptr }); ok { | |
| // if stderr is not a tty, simply print the error without any | |
| // styling or going through an [ErrorHandler]: | |
| if !term.IsTerminal(w.Fd()) { | |
| fmt.Fprintln(w, err.Error()) | |
| return err | |
| } | |
| } else if f, ok := root.ErrOrStderr().(*os.File); ok { | |
| // fallback check for *os.File | |
| if !term.IsTerminal(f.Fd()) { | |
| fmt.Fprintln(f, err.Error()) | |
| return err | |
| } |
Copilot uses AI. Check for mistakes.
| // ErrorHandler handles an error, printing them to the given [io.Writer]. | ||
| // | ||
| // Note that this will only be used if the STDERR is a terminal, and should | ||
| // be used for styling only. | ||
| type ErrorHandler = func(w io.Writer, styles Styles, err error) |
There was a problem hiding this comment.
Two remarks
-
Is there a need for this to be exported?
-
you are not using it in your PR, right? Except in a comment I don't see it.
Why having it then?
|
What an elegant solution thanks for implementing |
fixes #50