Skip to content

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.

Sign up for GitHub

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

Merged
merged 23 commits into from
Sep 17, 2024
Merged

fix: use ANSI colors codes instead of RGB #14665

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
5e308df
chore: add command for showing colors
DanielleMaywood Sep 13, 2024
86175bf
fix: use ANSI color codes instead of RGB
DanielleMaywood Sep 13, 2024
098d35d
feat: add '--no-color' flag
DanielleMaywood Sep 13, 2024
3d13060
fix: revert colors
DanielleMaywood Sep 13, 2024
5b8fa3b
chore: change colors
DanielleMaywood Sep 13, 2024
4ce84a4
fix: update golden files
DanielleMaywood Sep 13, 2024
a75dbb2
fix: replace blue with brightBlue
DanielleMaywood Sep 13, 2024
390a7ca
fix: drop '> ' for unfocused prompts
DanielleMaywood Sep 13, 2024
7e6db79
fix: run 'make fmt'
DanielleMaywood Sep 13, 2024
ce913a5
chore: allow disabling color with env flags
DanielleMaywood Sep 13, 2024
22a2d0b
fix: apply fixes from feedback
DanielleMaywood Sep 13, 2024
5e42118
fix: run 'make gen'
DanielleMaywood Sep 13, 2024
40eb24e
fix: refactor janky code
DanielleMaywood Sep 13, 2024
ccf174a
fix: re-add public function
DanielleMaywood Sep 13, 2024
1fbef1d
fix: re-add init for non-color tests
DanielleMaywood Sep 13, 2024
44c3726
fix: move styles to 'init' that can be
DanielleMaywood Sep 13, 2024
d16f625
fix: stop overwriting entire DefaultStyles
DanielleMaywood Sep 13, 2024
1babe96
fix: make code and field obey --no-color
DanielleMaywood Sep 13, 2024
ff3c392
fix: rip out '--no-color' due to race condition
DanielleMaywood Sep 13, 2024
6d93c56
fix: apply nit
DanielleMaywood Sep 13, 2024
506aa28
fix: simplify code && hide command
DanielleMaywood Sep 16, 2024
997dc43
fix: newline shouldn't be themed
DanielleMaywood Sep 16, 2024
24a838d
fix: appease the linter
DanielleMaywood Sep 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
feat: add '--no-color' flag
  • Loading branch information
@DanielleMaywood
DanielleMaywood committed Sep 13, 2024
commit 098d35d7d44b3c50c8bf0bbeaafbea850eab1cc2
23 changes: 19 additions & 4 deletions cli/cliui/cliui.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@ package cliui

import (
"flag"
"fmt"
"os"
"slices"
"sync"
"time"

Expand All @@ -12,6 +14,10 @@ import (
"github.com/coder/pretty"
)

const NoColorFlag = "no-color"

var NoColor = false

var Canceled = xerrors.New("canceled")

// DefaultStyles compose visual elements of the UI.
Expand All @@ -37,21 +43,30 @@ var (
)

var (
Green = Color("2")
Red = Color("1")
Yellow = Color("3")
Blue = Color("6")
Green = Color("10")
Red = Color("9")
Yellow = Color("11")
Blue = Color("12")
)

// Color returns a color for the given string.
func Color(s string) termenv.Color {
colorOnce.Do(func() {
color = termenv.NewOutput(os.Stdout).ColorProfile()

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
Copy link
Member

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.

// 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)) {
color = termenv.Ascii
}
Copy link
Member

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 checking if NoColor should suffice?

That said, I really wish we could do it differently as modifying globals is a bit icky 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately not as NoColor is set after our code runs. This is because there is no (apparent) way to have this initialization code ran before every command but after serpent sets up the options.

})
return color.Color(s)
}
Expand Down
6 changes: 6 additions & 0 deletions cli/root.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,6 +461,12 @@ func (r *RootCmd) Command(subcommands []*serpent.Command) (*serpent.Command, err
Value: serpent.StringOf(&r.globalConfig),
Group: globalGroup,
},
{
Flag: cliui.NoColorFlag,
Copy link
Member

@johnstcn johnstcn Sep 13, 2024 •
edited
Loading

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.

Copy link
Contributor Author

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 using NO_COLOR for this https://no-color.org. We could do both NO_COLOR and CODER_NO_COLOR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me!

Copy link
Member

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.

Description: "Disable use of color in CLI output.",
Value: serpent.BoolOf(&cliui.NoColor),
Group: globalGroup,
},
{
Flag: "version",
// This was requested by a customer to assist with their migration.
Expand Down
8 changes: 8 additions & 0 deletions cmd/cliui/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,14 @@ func main() {
},
}

root.Options = []serpent.Option{
{
Default: "false",
Flag: cliui.NoColorFlag,
Value: serpent.BoolOf(&cliui.NoColor),
},
}

root.Children = append(root.Children, &serpent.Command{
Use: "colors",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be hidden

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

Handler: func(inv *serpent.Invocation) error {
Expand Down