Implement terminal color using wrapper around io.Writer#856
Closed
bobcatfish wants to merge 1 commit intoGoogleContainerTools:masterfrom
Closed
Implement terminal color using wrapper around io.Writer#856bobcatfish wants to merge 1 commit intoGoogleContainerTools:masterfrom
bobcatfish wants to merge 1 commit intoGoogleContainerTools:masterfrom
Conversation
In GoogleContainerTools#850 I proposed an implementation of terminal color which involves instantiating a formatter object whenever you want to log to the terminal in color. This PR instead creates a wrapper around io.Writer instead, so that theoretically everything output to the terminal will magically be the right color without callers needing to be aware. That was how it was in the first iteration HOWEVER when I ran `skaffold dev` _everything_ was blue, including output from docker, etc. It became clear that we needed to often use the output writer which didn't apply formatting, so in the end we need to change pretty much every function that took io.Writer to take color.Writer instead, so we can control whether the functions they call output in color or not. The end result is that when you look at some code, if it still takes io.Writer as a param, it's very hard to know if that will be in color or not and I feel this implementation is more confusing and less maintainable than GoogleContainerTools#850. (This was a possible fix for GoogleContainerTools#835.)
Contributor
Author
|
Initially I didn't like this idea (when discussed with @r2d4) but after looking at similar libs (such as |
bobcatfish
added a commit
to bobcatfish/skaffold
that referenced
this pull request
Jul 27, 2018
This implementation avoids needing to instantiate a formatter object (as seen in GoogleContainerTools#850) by using a global function pointer to override the IsTerminal logic. This means any code at any time could output in whatever color it wants to. (Thanks @r2d4 for suggesting this implementation in the first place!) Skaffold draws a distinction between log output, which goes to stdout, and output from Skaffold to the user, which goes to stdout and is interleaved with output from the tools Skaffold users (e.g. docker for building). This commit changes the output from Skaffold to the user to be Blue! The escape codes to make the text blue (or other colors, in the case of the kube pod-specific logging) will only be output if stdout is a terminal. If output is being redirected to a file for example, the escape codes will not be included. This change makes use of the exciting colors.go logic but splits it into the picker (specifically for mapping images to colors consistently) from the logic to add escape codes. Other alternatives: * GoogleContainerTools#856 - Wrap io.Writer * GoogleContainerTools#850 - Instantiate a formatter object * Separate formatting logic from writing logic (I tried this out and it didn't seem like it improved anything as it didn't align well with how `fmt` is implemented)
This was referenced Jul 27, 2018
bobcatfish
added a commit
to bobcatfish/skaffold
that referenced
this pull request
Jul 27, 2018
This implementation avoids needing to instantiate a formatter object (as seen in GoogleContainerTools#850) by using a global function pointer to override the IsTerminal logic. This means any code at any time could output in whatever color it wants to. (Thanks @r2d4 for suggesting this implementation in the first place!) Skaffold draws a distinction between log output, which goes to stdout, and output from Skaffold to the user, which goes to stdout and is interleaved with output from the tools Skaffold users (e.g. docker for building). This commit changes the output from Skaffold to the user to be Blue! The escape codes to make the text blue (or other colors, in the case of the kube pod-specific logging) will only be output if stdout is a terminal. If output is being redirected to a file for example, the escape codes will not be included. This change makes use of the exciting colors.go logic but splits it into the picker (specifically for mapping images to colors consistently) from the logic to add escape codes. Other alternatives: * GoogleContainerTools#856 - Wrap io.Writer * GoogleContainerTools#850 - Instantiate a formatter object * Separate formatting logic from writing logic (I tried this out and it didn't seem like it improved anything as it didn't align well with how `fmt` is implemented)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
(I don't want to actually propose this as a solution but I want to make a PR to demonstrate what this proposal would look like)
In #850 I proposed an implementation of terminal color which involves
instantiating a formatter object whenever you want to log to the
terminal in color. This PR instead creates a wrapper around io.Writer
instead, so that theoretically everything output to the terminal will
magically be the right color without callers needing to be aware.
That was how it was in the first iteration HOWEVER when I ran
skaffold deveverything was blue, including output from docker, etc. It becameclear that we needed to often use the output writer which didn't apply
formatting, so in the end we need to change pretty much every function
that took io.Writer to take color.Writer instead, so we can control
whether the functions they call output in color or not.
The end result is that when you look at some code, if it still takes
io.Writer as a param, it's very hard to know if that will be in color or
not and I feel this implementation is more confusing and less
maintainable than #850.
(This was a possible fix for #835.)