New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow to use io.Writer instead of *os.File for NewColorable func #66
base: master
Are you sure you want to change the base?
Conversation
// FIXME: remove, after merge of https://github.com/mattn/go-isatty/pull/81 | ||
replace github.com/mattn/go-isatty => github.com/cardil/mattn-isatty v0.0.0-20230228121058-48002435730c |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this before merge
func setColorable(w io.Writer) io.Writer {
if f, isFile := w.(*os.File); isFile {
return colorable.NewColorable(f)
}
return w
} So 👎 |
Can you explain how it breaks the API? I'm changing here the parameter type from |
Hey @cardil, thanks for the PR and @dolmen, thanks for reviewing. First, to answer your question @cardil: changing the function signature is a breaking change. someFunc(colorable.NewColorable)
// where
func someFunc(newColorable func(*os.File) io.Writer) {
// ...
} That code will now stop compiling because @dolmen, unfortunately, that alternative won't suffice because it still requires a specific implementation ( To do this in a backwards compatible manner, this needs to be a new function. As a suggestion, consider: func NewColorableWriter(w io.Writer) io.Writer {
// ...
} This could be made easier by putting the existing NewColorable function into a regular file (not tagged with a platform-specific build tag), that just calls the individual platform-specific NewColorableWriter function. Roughly: // colorable.go
func NewColorable(f *os.File) io.Writer {
return NewColorableWriter(f)
}
// colorable_windows.go and colorable_others.go
func NewColorableWriter(w io.Writer) io.Writer {
// ...
} This change would be completely backwards-compatible and support this functionality on io.Writers. |
Fixes #65
Require mattn/go-isatty#81