- Notifications
You must be signed in to change notification settings - Fork928
fix: use ANSI colors codes instead of RGB#14665
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
d97280b to4ce84a4CompareThere 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.
Nice work! Just some nits on my end.
Smoke-tested example prompts with and without colour.
cli/root.go Outdated
| @@ -461,6 +461,12 @@ func (r *RootCmd) Command(subcommands []*serpent.Command) (*serpent.Command, err | |||
| Value: serpent.StringOf(&r.globalConfig), | |||
| Group: globalGroup, | |||
| }, | |||
| { | |||
| Flag: cliui.NoColorFlag, | |||
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.
Wonder if it would make sense to also allow setting it as an env (CODER_NO_COLOR)? Not blocking, but it would allow folks for whom our colour scheme doesn't work to simply turn it off forever by setting it in their shell profile.
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.
Yes that should be a good idea. There is a 'standard' around usingNO_COLOR for thishttps://no-color.org. We could do bothNO_COLOR andCODER_NO_COLOR?
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.
Sounds good to me!
| red = Color("1") | ||
| green = Color("2") | ||
| yellow = Color("3") | ||
| magenta = Color("5") | ||
| white = Color("7") | ||
| brightBlue = Color("12") | ||
| brightMagenta = Color("13") |
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.
nit: indicate these are ANSI color codes
cli/cliui/cliui.go Outdated
| func ifTerm(f pretty.Formatter) pretty.Formatter { | ||
| if !isTerm() { | ||
| return pretty.Nop | ||
| } | ||
| returnfmt | ||
| returnf |
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.
👍
cmd/cliui/main.go Outdated
| } | ||
| root.Children = append(root.Children, &serpent.Command{ | ||
| Use: "colors", |
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.
👍 👍
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.
I think the global NoColor var could cause a race in our testing since we run things concurrently. E.g. if multiple tests incli package set--no-color option, we'd run into it.
Other than that, I think this looks great. Might be worth looking into improving serpent so we can do this in a better way. And perhaps get rid of the package-global state in favor of a "color" instance. That's a much more intrusive change though.
cli/root.go Outdated
| @@ -461,6 +461,12 @@ func (r *RootCmd) Command(subcommands []*serpent.Command) (*serpent.Command, err | |||
| Value: serpent.StringOf(&r.globalConfig), | |||
| Group: globalGroup, | |||
| }, | |||
| { | |||
| Flag: cliui.NoColorFlag, | |||
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.
Suggestion: I think we should add the env as well here.
cli/cliui/cliui.go Outdated
| // has the flag. | ||
| if slices.Contains(os.Args, fmt.Sprintf("--%s", NoColorFlag)) { | ||
| color = termenv.Ascii | ||
| } |
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.
Is this needed? I see that the serpent options modify the global var in this package, so checkingif NoColor should suffice?
That said, I really wish we could do it differently as modifying globals is a bit icky 😅
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.
Unfortunately not asNoColor is setafter our code runs. This is because there is no (apparent) way to have this initialization code ran beforeevery command but after serpent sets up the options.
cli/cliui/cliui.go Outdated
| pretty.FgColor(Red), | ||
| pretty.BgColor(color.Color("#2c2c2c")), | ||
| pretty.FgColor(color.Color("#ED567A")), | ||
| pretty.BgColor(color.Color("#2C2C2C")), |
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.
Were there no acceptable ANSI colors to replace these? Why did we go from red to ed567a?
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.
As we set both the foreground and background colour, we have complete control over the contrast so this looks the same regardless of the terminal theme. If we want to use an ANSI code instead we can but there isn't a need to here.
red was defined as#ED567A, and the newred didn't look as good on the background so I kept it as the old one.
@mafredri potentially, yes -- but we already check for |
I should probably have documented this but we don't actually use the global var, I needed it to set it in We can't make use of it as serpent sets it too late as we can't hook into serpent before it run commands. |
mafredri commentedSep 13, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Updating the var from multiple locations is still a data race, can be seen in CI here:https://github.com/coder/coder/actions/runs/10851466684/job/30115309148?pr=14665#step:5:5121 Edit: I just checked the first instance of data race and linked it, stack trace doesn't really say that it's due to the global var AFAICT 😂 |
mafredri commentedSep 13, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Alright, then I think using a local var to the serpent command would be better. 👍🏻 (+ comment explaining it's purpose) |
Turns out serpent behaves differently with |
So define a local var in |
cli/cliui/cliui.go Outdated
| // serpent as it isn't possible to create a root middleware that | ||
| // runs for every command. For now we just check if `os.Args` | ||
| // has the flag. | ||
| if slices.Contains(os.Args, fmt.Sprintf("--%s", NoColorFlag)) || |
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.
This should also support--no-color=true and--no-color=true (omitting= means true) to be consistent with our CLI. Ideally we'd rely on serpent parsing here but I understand that not easily achieved.
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.
Can do!
--no-color=trueand--no-color=true
Do my eyes deceive me or are these both the same?
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.
A case of copy-pasta 😂, sorry. Meantfalse for the second.
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.
Got you, just pushed a change with--no-color=true handling. It appears we already handle the--no-color=false case!
Yep, inside |
cli/cliui/cliui.go Outdated
| if flag.Lookup("test.v") != nil { | ||
| // Use a consistent colorless profile in tests so that results | ||
| // are deterministic. | ||
| color = termenv.Ascii | ||
| } | ||
| // Currently it appears there is no way to use the flag from |
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.
Have you tried setting the option on the root command? It should then evaluate on every command.
cli/cliui/cliui.go Outdated
| // serpent as it isn't possible to create a root middleware that | ||
| // runs for every command. For now we just check if `os.Args` | ||
| // has the flag. | ||
| if slices.Contains(os.Args, fmt.Sprintf("--%s", NoColorFlag)) || |
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.
This is super janky. Relying on os.Args makes it difficult to test. Also, someone could modify the option at the CLI layer and easily forget to update this. Have you considered checking the CLI option higher up in the stack?
DanielleMaywoodSep 13, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
I absolutely agree with you, however serpent doesn't (from what I can tell) have any way to support this as it lacks the ability to set a middleware that runs beforeany command.
To demonstrate what I mean, the code inthis gist has aMiddleware setup but it only runs on the root cmd and not it's children. If you know of a way to have this run before a commands handler but after options are setup I'd love to do that instead!
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.
Scratch that: I think usingrootCmd.Walk and injecting the middleware to every child should work? Will have a go at doing that.
DanielleMaywoodSep 13, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
I've pushed a change that should be less janky (using the method describe in the prior message), let me know what you think!
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.
If you do the Walk approach we have the flag duplicated on every help, which is obnoxious. Can't you use an Option on the root command?
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.
Sorry I think I explained myself poorly, the Walk was to inject middleware onto every command. The Option was on the root command. This change was backed out anyways as mutating theDefaultStyles afterfunc init() was causing Go's race checker to be unhappy.
We still support `NO_COLOR` env variable through termenv's`EnvColorProfile`. The reason for the race condition is that`DefaultStyles` is a global that we shouldn't mutate after `init`is called, but we have to mutate it after `init` has ran to haveserpent collect the cli flags and env vars for us.
I've ripped out all the logic for |
cmd/cliui/main.go Outdated
| @@ -37,6 +38,43 @@ func main() { | |||
| }, | |||
| } | |||
| root.Children = append(root.Children, &serpent.Command{ | |||
| Use: "colors", | |||
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.
This should be hidden
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.
Done!
cmd/cliui/main.go Outdated
| Use: "colors", | ||
| Handler: func(inv *serpent.Invocation) error { | ||
| msg := pretty.Sprint(cliui.DefaultStyles.Code, "This is a code message") | ||
| _, _ = fmt.Fprintln(inv.Stdout, msg) |
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.
You can simplify this withpretty.Fprintf
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.
Done! I had to separate the newline from the Fprintf call though as otherwise the newline would get themed.
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.
Should prob only merge when one of the core devs approve. I generally don't want to take accountability as a review since my knowledge of the internals has atrophied.
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.
Looks great. I think relying on ansi colors where we don't use a background, and hex colors where we do, is indeed the simplest approach.
Thanks for working through all the iterations of this and glad we could land on a simple solution in the end. 👍🏻
| ) | ||
| // Color returns a color for the given string. | ||
| func Color(s string) termenv.Color { | ||
| colorOnce.Do(func() { | ||
| color = termenv.NewOutput(os.Stdout).ColorProfile() | ||
| color = termenv.NewOutput(os.Stdout).EnvColorProfile() |
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.
Good solution for disabling colors 👍🏻
14d3e30 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Fixes#13211
Instead of using RGB color codes, we instead use ANSI color codes. This means that we respect a user's terminal theme. Not all of the new colors match the old ones but this is a limitation with using 4-bit ANSI colors.
This change also allows users to pass
--no-colorto the CLI to completely disable theming. It is worth noting that whilst we do define theserpent.Option, we manually checkos.Argsfor the flag as there doesn't appear to be a way to have a global middleware withserpent(whereasspf13/cobradoes have this).This change also removes
>from Prompt, and replaces it withas on--no-colorthere is no difference.Before:
After:
Before:
After:
Before:
After:
Before:
After:
WithNow requires--no-colorNO_COLORenvironment variable