From 3eca268a7f60f909afe8ad4cb77b2708d94a2525 Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Wed, 2 Apr 2025 18:24:20 -0400 Subject: [PATCH 1/6] Introduce `color_labels` support, update commands This commit implements the actual changes around configuration setting / environment variable logic for displaying labels using their RGB hex color code in terminals with truecolor support. One of the subtler changes in this commit is renaming generic ColorScheme.HexToRGB logic to render truecolor to ColorScheme.Label as this feature was being used exclusively for labels. This is due to confusion about introducing the new `color_labels` config on top of generic coloring logic. --- internal/config/config.go | 17 ++++++++ internal/config/config_test.go | 2 + internal/config/stub.go | 3 ++ internal/gh/gh.go | 2 + internal/gh/mock/config.go | 44 +++++++++++++++++++ pkg/cmd/config/list/list_test.go | 19 +++++---- pkg/cmd/factory/default.go | 6 +++ pkg/cmd/factory/default_test.go | 47 ++++++++++++++++++++ pkg/cmd/gist/list/list_test.go | 2 +- pkg/cmd/issue/shared/display.go | 2 +- pkg/cmd/issue/view/view.go | 2 +- pkg/cmd/label/list.go | 7 ++- pkg/cmd/pr/view/view.go | 2 +- pkg/cmd/search/shared/shared.go | 2 +- pkg/iostreams/color.go | 20 +++------ pkg/iostreams/color_test.go | 73 ++++++++------------------------ pkg/iostreams/iostreams.go | 11 ++++- 17 files changed, 178 insertions(+), 83 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 476154d667e..e7534dfdb5f 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -17,6 +17,7 @@ import ( const ( aliasesKey = "aliases" browserKey = "browser" + colorLabelsKey = "color_labels" editorKey = "editor" gitProtocolKey = "git_protocol" hostsKey = "hosts" @@ -113,6 +114,11 @@ func (c *cfg) Browser(hostname string) gh.ConfigEntry { return c.GetOrDefault(hostname, browserKey).Unwrap() } +func (c *cfg) ColorLabels(hostname string) gh.ConfigEntry { + // Intentionally panic if there is no user provided value or default value (which would be a programmer error) + return c.GetOrDefault(hostname, colorLabelsKey).Unwrap() +} + func (c *cfg) Editor(hostname string) gh.ConfigEntry { // Intentionally panic if there is no user provided value or default value (which would be a programmer error) return c.GetOrDefault(hostname, editorKey).Unwrap() @@ -532,6 +538,8 @@ aliases: http_unix_socket: # What web browser gh should use when opening URLs. If blank, will refer to environment. browser: +# Whether to display labels using their RGB hex color codes in terminals that support truecolor. Supported values: enabled, disabled +color_labels: disabled ` type ConfigOption struct { @@ -602,6 +610,15 @@ var Options = []ConfigOption{ return c.Browser(hostname).Value }, }, + { + Key: colorLabelsKey, + Description: "whether to display labels using their RGB hex color codes in terminals that support truecolor", + DefaultValue: "disabled", + AllowedValues: []string{"enabled", "disabled"}, + CurrentValue: func(c gh.Config, hostname string) string { + return c.ColorLabels(hostname).Value + }, + }, } func HomeDirPath(subdir string) (string, error) { diff --git a/internal/config/config_test.go b/internal/config/config_test.go index fef87ddc644..67a9a98d1ab 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -32,6 +32,7 @@ func TestNewConfigProvidesFallback(t *testing.T) { requireKeyWithValue(t, spiedCfg, []string{aliasesKey, "co"}, "pr checkout") requireKeyWithValue(t, spiedCfg, []string{httpUnixSocketKey}, "") requireKeyWithValue(t, spiedCfg, []string{browserKey}, "") + requireKeyWithValue(t, spiedCfg, []string{colorLabelsKey}, "disabled") } func TestGetOrDefaultApplicationDefaults(t *testing.T) { @@ -137,6 +138,7 @@ func TestFallbackConfig(t *testing.T) { requireKeyWithValue(t, cfg, []string{aliasesKey, "co"}, "pr checkout") requireKeyWithValue(t, cfg, []string{httpUnixSocketKey}, "") requireKeyWithValue(t, cfg, []string{browserKey}, "") + requireKeyWithValue(t, cfg, []string{colorLabelsKey}, "disabled") requireNoKey(t, cfg, []string{"unknown"}) } diff --git a/internal/config/stub.go b/internal/config/stub.go index 71d44556dc1..78073da4a17 100644 --- a/internal/config/stub.go +++ b/internal/config/stub.go @@ -55,6 +55,9 @@ func NewFromString(cfgStr string) *ghmock.ConfigMock { mock.BrowserFunc = func(hostname string) gh.ConfigEntry { return cfg.Browser(hostname) } + mock.ColorLabelsFunc = func(hostname string) gh.ConfigEntry { + return cfg.ColorLabels(hostname) + } mock.EditorFunc = func(hostname string) gh.ConfigEntry { return cfg.Editor(hostname) } diff --git a/internal/gh/gh.go b/internal/gh/gh.go index 8e640c41a88..8f94d7f0d88 100644 --- a/internal/gh/gh.go +++ b/internal/gh/gh.go @@ -37,6 +37,8 @@ type Config interface { // Browser returns the configured browser, optionally scoped by host. Browser(hostname string) ConfigEntry + // ColorLabels returns the configured colorize labels setting, optionally scoped by host. + ColorLabels(hostname string) ConfigEntry // Editor returns the configured editor, optionally scoped by host. Editor(hostname string) ConfigEntry // GitProtocol returns the configured git protocol, optionally scoped by host. diff --git a/internal/gh/mock/config.go b/internal/gh/mock/config.go index 569af1facb4..b94cb084dc3 100644 --- a/internal/gh/mock/config.go +++ b/internal/gh/mock/config.go @@ -31,6 +31,9 @@ var _ gh.Config = &ConfigMock{} // CacheDirFunc: func() string { // panic("mock out the CacheDir method") // }, +// ColorLabelsFunc: func(hostname string) gh.ConfigEntry { +// panic("mock out the ColorLabels method") +// }, // EditorFunc: func(hostname string) gh.ConfigEntry { // panic("mock out the Editor method") // }, @@ -83,6 +86,9 @@ type ConfigMock struct { // CacheDirFunc mocks the CacheDir method. CacheDirFunc func() string + // ColorLabelsFunc mocks the ColorLabels method. + ColorLabelsFunc func(hostname string) gh.ConfigEntry + // EditorFunc mocks the Editor method. EditorFunc func(hostname string) gh.ConfigEntry @@ -132,6 +138,11 @@ type ConfigMock struct { // CacheDir holds details about calls to the CacheDir method. CacheDir []struct { } + // ColorLabels holds details about calls to the ColorLabels method. + ColorLabels []struct { + // Hostname is the hostname argument value. + Hostname string + } // Editor holds details about calls to the Editor method. Editor []struct { // Hostname is the hostname argument value. @@ -194,6 +205,7 @@ type ConfigMock struct { lockAuthentication sync.RWMutex lockBrowser sync.RWMutex lockCacheDir sync.RWMutex + lockColorLabels sync.RWMutex lockEditor sync.RWMutex lockGetOrDefault sync.RWMutex lockGitProtocol sync.RWMutex @@ -320,6 +332,38 @@ func (mock *ConfigMock) CacheDirCalls() []struct { return calls } +// ColorLabels calls ColorLabelsFunc. +func (mock *ConfigMock) ColorLabels(hostname string) gh.ConfigEntry { + if mock.ColorLabelsFunc == nil { + panic("ConfigMock.ColorLabelsFunc: method is nil but Config.ColorLabels was just called") + } + callInfo := struct { + Hostname string + }{ + Hostname: hostname, + } + mock.lockColorLabels.Lock() + mock.calls.ColorLabels = append(mock.calls.ColorLabels, callInfo) + mock.lockColorLabels.Unlock() + return mock.ColorLabelsFunc(hostname) +} + +// ColorLabelsCalls gets all the calls that were made to ColorLabels. +// Check the length with: +// +// len(mockedConfig.ColorLabelsCalls()) +func (mock *ConfigMock) ColorLabelsCalls() []struct { + Hostname string +} { + var calls []struct { + Hostname string + } + mock.lockColorLabels.RLock() + calls = mock.calls.ColorLabels + mock.lockColorLabels.RUnlock() + return calls +} + // Editor calls EditorFunc. func (mock *ConfigMock) Editor(hostname string) gh.ConfigEntry { if mock.EditorFunc == nil { diff --git a/pkg/cmd/config/list/list_test.go b/pkg/cmd/config/list/list_test.go index 65f83d659c3..2184d0f1659 100644 --- a/pkg/cmd/config/list/list_test.go +++ b/pkg/cmd/config/list/list_test.go @@ -4,6 +4,7 @@ import ( "bytes" "testing" + "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/internal/gh" "github.com/cli/cli/v2/pkg/cmdutil" @@ -91,14 +92,16 @@ func Test_listRun(t *testing.T) { return cfg }(), input: &ListOptions{Hostname: "HOST"}, - stdout: `git_protocol=ssh -editor=/usr/bin/vim -prompt=disabled -prefer_editor_prompt=enabled -pager=less -http_unix_socket= -browser=brave -`, + stdout: heredoc.Doc(` + git_protocol=ssh + editor=/usr/bin/vim + prompt=disabled + prefer_editor_prompt=enabled + pager=less + http_unix_socket= + browser=brave + color_labels=disabled + `), }, } diff --git a/pkg/cmd/factory/default.go b/pkg/cmd/factory/default.go index 9236514877c..5e9c25ab29b 100644 --- a/pkg/cmd/factory/default.go +++ b/pkg/cmd/factory/default.go @@ -292,6 +292,12 @@ func ioStreams(f *cmdutil.Factory) *iostreams.IOStreams { io.SetPager(pager.Value) } + if _, ghColorLabels := os.LookupEnv("GH_COLOR_LABELS"); ghColorLabels { + io.SetColorLabels(true) // TODO: should this be a truthy value? + } else if prompt := cfg.ColorLabels(""); prompt.Value == "enabled" { + io.SetColorLabels(true) + } + return io } diff --git a/pkg/cmd/factory/default_test.go b/pkg/cmd/factory/default_test.go index 94955bb30ec..b1730d6e615 100644 --- a/pkg/cmd/factory/default_test.go +++ b/pkg/cmd/factory/default_test.go @@ -432,6 +432,49 @@ func Test_ioStreams_prompt(t *testing.T) { } } +func Test_ioStreams_colorLabels(t *testing.T) { + tests := []struct { + name string + config gh.Config + colorLabelsEnabled bool + env map[string]string + }{ + { + name: "default config", + colorLabelsEnabled: false, + }, + { + name: "config with colorLabels enabled", + config: enableColorLabelsConfig(), + colorLabelsEnabled: true, + }, + { + name: "colorLabels enabled via GH_COLOR_LABELS env var", + env: map[string]string{"GH_COLOR_LABELS": "1"}, + colorLabelsEnabled: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.env != nil { + for k, v := range tt.env { + t.Setenv(k, v) + } + } + f := New("1") + f.Config = func() (gh.Config, error) { + if tt.config == nil { + return config.NewBlankConfig(), nil + } else { + return tt.config, nil + } + } + io := ioStreams(f) + assert.Equal(t, tt.colorLabelsEnabled, io.ColorLabels()) + }) + } +} + func TestSSOURL(t *testing.T) { tests := []struct { name string @@ -537,3 +580,7 @@ func pagerConfig() gh.Config { func disablePromptConfig() gh.Config { return config.NewFromString("prompt: disabled") } + +func enableColorLabelsConfig() gh.Config { + return config.NewFromString("color_labels: enabled") +} diff --git a/pkg/cmd/gist/list/list_test.go b/pkg/cmd/gist/list/list_test.go index 4f6c8a9f7e2..1b55478b300 100644 --- a/pkg/cmd/gist/list/list_test.go +++ b/pkg/cmd/gist/list/list_test.go @@ -694,7 +694,7 @@ func Test_highlightMatch(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - cs := iostreams.NewColorScheme(tt.color, false, false, iostreams.NoTheme) + cs := iostreams.NewColorScheme(tt.color, false, false, false, iostreams.NoTheme) matched := false got, err := highlightMatch(tt.input, regex, &matched, cs.Blue, cs.Highlight) diff --git a/pkg/cmd/issue/shared/display.go b/pkg/cmd/issue/shared/display.go index 0c56ffd2cae..bfb9fc2c646 100644 --- a/pkg/cmd/issue/shared/display.go +++ b/pkg/cmd/issue/shared/display.go @@ -56,7 +56,7 @@ func issueLabelList(issue *api.Issue, cs *iostreams.ColorScheme, colorize bool) labelNames := make([]string, 0, len(issue.Labels.Nodes)) for _, label := range issue.Labels.Nodes { if colorize { - labelNames = append(labelNames, cs.HexToRGB(label.Color, label.Name)) + labelNames = append(labelNames, cs.Label(label.Color, label.Name)) } else { labelNames = append(labelNames, label.Name) } diff --git a/pkg/cmd/issue/view/view.go b/pkg/cmd/issue/view/view.go index b188c6a4ca3..e2420a820e5 100644 --- a/pkg/cmd/issue/view/view.go +++ b/pkg/cmd/issue/view/view.go @@ -317,7 +317,7 @@ func issueLabelList(issue *api.Issue, cs *iostreams.ColorScheme) string { if cs == nil { labelNames[i] = label.Name } else { - labelNames[i] = cs.HexToRGB(label.Color, label.Name) + labelNames[i] = cs.Label(label.Color, label.Name) } } diff --git a/pkg/cmd/label/list.go b/pkg/cmd/label/list.go index fe1e9cdb7f6..4fe2f5ec9e3 100644 --- a/pkg/cmd/label/list.go +++ b/pkg/cmd/label/list.go @@ -137,7 +137,12 @@ func printLabels(io *iostreams.IOStreams, labels []label) error { table := tableprinter.New(io, tableprinter.WithHeader("NAME", "DESCRIPTION", "COLOR")) for _, label := range labels { - table.AddField(label.Name, tableprinter.WithColor(cs.ColorFromRGB(label.Color))) + // Colorize the label using tableprinter's WithColor function for it to handle non-TTY situations + labelColor := tableprinter.WithColor(func(s string) string { + return cs.Label(label.Color, s) + }) + + table.AddField(label.Name, labelColor) table.AddField(label.Description) table.AddField("#" + label.Color) diff --git a/pkg/cmd/pr/view/view.go b/pkg/cmd/pr/view/view.go index ccad4fa77cf..719a8fc7b66 100644 --- a/pkg/cmd/pr/view/view.go +++ b/pkg/cmd/pr/view/view.go @@ -423,7 +423,7 @@ func prLabelList(pr api.PullRequest, cs *iostreams.ColorScheme) string { labelNames := make([]string, 0, len(pr.Labels.Nodes)) for _, label := range pr.Labels.Nodes { - labelNames = append(labelNames, cs.HexToRGB(label.Color, label.Name)) + labelNames = append(labelNames, cs.Label(label.Color, label.Name)) } list := strings.Join(labelNames, ", ") diff --git a/pkg/cmd/search/shared/shared.go b/pkg/cmd/search/shared/shared.go index f0a346fc878..1e3d0069ba2 100644 --- a/pkg/cmd/search/shared/shared.go +++ b/pkg/cmd/search/shared/shared.go @@ -158,7 +158,7 @@ func listIssueLabels(issue *search.Issue, cs *iostreams.ColorScheme, colorize bo labelNames := make([]string, 0, len(issue.Labels)) for _, label := range issue.Labels { if colorize { - labelNames = append(labelNames, cs.HexToRGB(label.Color, label.Name)) + labelNames = append(labelNames, cs.Label(label.Color, label.Name)) } else { labelNames = append(labelNames, label.Name) } diff --git a/pkg/iostreams/color.go b/pkg/iostreams/color.go index 07fdcd79fa6..49f9496f5dc 100644 --- a/pkg/iostreams/color.go +++ b/pkg/iostreams/color.go @@ -41,12 +41,13 @@ var ( // NewColorScheme initializes color logic based on provided terminal capabilities. // Logic dealing with terminal theme detected, such as whether color is enabled, 8-bit color supported, true color supported, -// and terminal theme detected. -func NewColorScheme(enabled, is256enabled, trueColor bool, theme string) *ColorScheme { +// labels are colored, and terminal theme detected. +func NewColorScheme(enabled, is256enabled, trueColor, colorLabels bool, theme string) *ColorScheme { return &ColorScheme{ enabled: enabled, is256enabled: is256enabled, hasTrueColor: trueColor, + colorLabels: colorLabels, theme: theme, } } @@ -55,6 +56,7 @@ type ColorScheme struct { enabled bool is256enabled bool hasTrueColor bool + colorLabels bool theme string } @@ -240,17 +242,9 @@ func (c *ColorScheme) ColorFromString(s string) func(string) string { return fn } -// ColorFromRGB returns a function suitable for TablePrinter.AddField -// that calls HexToRGB, coloring text if supported by the terminal. -func (c *ColorScheme) ColorFromRGB(hex string) func(string) string { - return func(s string) string { - return c.HexToRGB(hex, s) - } -} - -// HexToRGB uses the given hex to color x if supported by the terminal. -func (c *ColorScheme) HexToRGB(hex string, x string) string { - if !c.enabled || !c.hasTrueColor || len(hex) != 6 { +// Label stylizes text based on label's RGB hex color. +func (c *ColorScheme) Label(hex string, x string) string { + if !c.enabled || !c.hasTrueColor || !c.colorLabels || len(hex) != 6 { return x } diff --git a/pkg/iostreams/color_test.go b/pkg/iostreams/color_test.go index b35c2eb73a8..c477891ce29 100644 --- a/pkg/iostreams/color_test.go +++ b/pkg/iostreams/color_test.go @@ -7,7 +7,7 @@ import ( "github.com/stretchr/testify/assert" ) -func TestColorFromRGB(t *testing.T) { +func TestLabel(t *testing.T) { tests := []struct { name string hex string @@ -20,77 +20,40 @@ func TestColorFromRGB(t *testing.T) { hex: "fc0303", text: "red", wants: "\033[38;2;252;3;3mred\033[0m", - cs: NewColorScheme(true, true, true, NoTheme), + cs: NewColorScheme(true, true, true, true, NoTheme), }, { name: "no truecolor", hex: "fc0303", text: "red", wants: "red", - cs: NewColorScheme(true, true, false, NoTheme), + cs: NewColorScheme(true, true, false, true, NoTheme), }, { name: "no color", hex: "fc0303", text: "red", wants: "red", - cs: NewColorScheme(false, false, false, NoTheme), + cs: NewColorScheme(false, false, false, true, NoTheme), }, { name: "invalid hex", hex: "fc0", text: "red", wants: "red", - cs: NewColorScheme(false, false, false, NoTheme), - }, - } - - for _, tt := range tests { - fn := tt.cs.ColorFromRGB(tt.hex) - assert.Equal(t, tt.wants, fn(tt.text)) - } -} - -func TestHexToRGB(t *testing.T) { - tests := []struct { - name string - hex string - text string - wants string - cs *ColorScheme - }{ - { - name: "truecolor", - hex: "fc0303", - text: "red", - wants: "\033[38;2;252;3;3mred\033[0m", - cs: NewColorScheme(true, true, true, NoTheme), - }, - { - name: "no truecolor", - hex: "fc0303", - text: "red", - wants: "red", - cs: NewColorScheme(true, true, false, NoTheme), + cs: NewColorScheme(false, false, false, true, NoTheme), }, { - name: "no color", + name: "no color labels", hex: "fc0303", text: "red", wants: "red", - cs: NewColorScheme(false, false, false, NoTheme), - }, - { - name: "invalid hex", - hex: "fc0", - text: "red", - wants: "red", - cs: NewColorScheme(false, false, false, NoTheme), + cs: NewColorScheme(true, true, true, false, NoTheme), }, } for _, tt := range tests { - output := tt.cs.HexToRGB(tt.hex, tt.text) + output := tt.cs.Label(tt.hex, tt.text) assert.Equal(t, tt.wants, output) } } @@ -109,61 +72,61 @@ func TestTableHeader(t *testing.T) { }{ { name: "when color is disabled, text is not stylized", - cs: NewColorScheme(false, false, false, NoTheme), + cs: NewColorScheme(false, false, false, false, NoTheme), input: "this should not be stylized", expected: "this should not be stylized", }, { name: "when 4-bit color is enabled but no theme, 4-bit default color and underline are used", - cs: NewColorScheme(true, false, false, NoTheme), + cs: NewColorScheme(true, false, false, false, NoTheme), input: "this should have no explicit color but underlined", expected: fmt.Sprintf("%sthis should have no explicit color but underlined%s", defaultUnderline, reset), }, { name: "when 4-bit color is enabled and theme is light, 4-bit dark color and underline are used", - cs: NewColorScheme(true, false, false, LightTheme), + cs: NewColorScheme(true, false, false, false, LightTheme), input: "this should have dark foreground color and underlined", expected: fmt.Sprintf("%sthis should have dark foreground color and underlined%s", brightBlackUnderline, reset), }, { name: "when 4-bit color is enabled and theme is dark, 4-bit light color and underline are used", - cs: NewColorScheme(true, false, false, DarkTheme), + cs: NewColorScheme(true, false, false, false, DarkTheme), input: "this should have light foreground color and underlined", expected: fmt.Sprintf("%sthis should have light foreground color and underlined%s", dimBlackUnderline, reset), }, { name: "when 8-bit color is enabled but no theme, 4-bit default color and underline are used", - cs: NewColorScheme(true, true, false, NoTheme), + cs: NewColorScheme(true, true, false, false, NoTheme), input: "this should have no explicit color but underlined", expected: fmt.Sprintf("%sthis should have no explicit color but underlined%s", defaultUnderline, reset), }, { name: "when 8-bit color is enabled and theme is light, 4-bit dark color and underline are used", - cs: NewColorScheme(true, true, false, LightTheme), + cs: NewColorScheme(true, true, false, false, LightTheme), input: "this should have dark foreground color and underlined", expected: fmt.Sprintf("%sthis should have dark foreground color and underlined%s", brightBlackUnderline, reset), }, { name: "when 8-bit color is true and theme is dark, 4-bit light color and underline are used", - cs: NewColorScheme(true, true, false, DarkTheme), + cs: NewColorScheme(true, true, false, false, DarkTheme), input: "this should have light foreground color and underlined", expected: fmt.Sprintf("%sthis should have light foreground color and underlined%s", dimBlackUnderline, reset), }, { name: "when 24-bit color is enabled but no theme, 4-bit default color and underline are used", - cs: NewColorScheme(true, true, true, NoTheme), + cs: NewColorScheme(true, true, true, false, NoTheme), input: "this should have no explicit color but underlined", expected: fmt.Sprintf("%sthis should have no explicit color but underlined%s", defaultUnderline, reset), }, { name: "when 24-bit color is enabled and theme is light, 4-bit dark color and underline are used", - cs: NewColorScheme(true, true, true, LightTheme), + cs: NewColorScheme(true, true, true, false, LightTheme), input: "this should have dark foreground color and underlined", expected: fmt.Sprintf("%sthis should have dark foreground color and underlined%s", brightBlackUnderline, reset), }, { name: "when 24-bit color is true and theme is dark, 4-bit light color and underline are used", - cs: NewColorScheme(true, true, true, DarkTheme), + cs: NewColorScheme(true, true, true, false, DarkTheme), input: "this should have light foreground color and underlined", expected: fmt.Sprintf("%sthis should have light foreground color and underlined%s", dimBlackUnderline, reset), }, diff --git a/pkg/iostreams/iostreams.go b/pkg/iostreams/iostreams.go index 6c12d7911a6..e78575b9ce8 100644 --- a/pkg/iostreams/iostreams.go +++ b/pkg/iostreams/iostreams.go @@ -72,6 +72,7 @@ type IOStreams struct { colorOverride bool colorEnabled bool + colorLabels bool pagerCommand string pagerProcess *os.Process @@ -102,6 +103,10 @@ func (s *IOStreams) HasTrueColor() bool { return s.term.IsTrueColorSupported() } +func (s *IOStreams) ColorLabels() bool { + return s.colorLabels +} + // DetectTerminalTheme is a utility to call before starting the output pager so that the terminal background // can be reliably detected. func (s *IOStreams) DetectTerminalTheme() { @@ -134,6 +139,10 @@ func (s *IOStreams) SetColorEnabled(colorEnabled bool) { s.colorEnabled = colorEnabled } +func (s *IOStreams) SetColorLabels(colorLabels bool) { + s.colorLabels = colorLabels +} + func (s *IOStreams) SetStdinTTY(isTTY bool) { s.stdinTTYOverride = true s.stdinIsTTY = isTTY @@ -366,7 +375,7 @@ func (s *IOStreams) TerminalWidth() int { } func (s *IOStreams) ColorScheme() *ColorScheme { - return NewColorScheme(s.ColorEnabled(), s.ColorSupport256(), s.HasTrueColor(), s.TerminalTheme()) + return NewColorScheme(s.ColorEnabled(), s.ColorSupport256(), s.HasTrueColor(), s.ColorLabels(), s.TerminalTheme()) } func (s *IOStreams) ReadUserFile(fn string) ([]byte, error) { From 5f03c208a1d1dcd862b62d4497fdbfe91f486e77 Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Wed, 2 Apr 2025 18:32:37 -0400 Subject: [PATCH 2/6] Fix comment language --- internal/gh/gh.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/gh/gh.go b/internal/gh/gh.go index 8f94d7f0d88..b17c6bd67fb 100644 --- a/internal/gh/gh.go +++ b/internal/gh/gh.go @@ -37,7 +37,7 @@ type Config interface { // Browser returns the configured browser, optionally scoped by host. Browser(hostname string) ConfigEntry - // ColorLabels returns the configured colorize labels setting, optionally scoped by host. + // ColorLabels returns the configured color_label setting, optionally scoped by host. ColorLabels(hostname string) ConfigEntry // Editor returns the configured editor, optionally scoped by host. Editor(hostname string) ConfigEntry From addbc6ac5c0dc3ed4611904972d5b122e4426756 Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Fri, 4 Apr 2025 11:02:44 -0400 Subject: [PATCH 3/6] Add label color env var to help topic, unused fix This commit adds the new environment variable to the `gh environment` help topic. Additionally, there is a small fix for Go linter for an unused variable raised as a problem. --- pkg/cmd/root/help.go | 2 +- pkg/cmd/root/help_topic.go | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/root/help.go b/pkg/cmd/root/help.go index a7daa7b847f..7f8fb1c2e86 100644 --- a/pkg/cmd/root/help.go +++ b/pkg/cmd/root/help.go @@ -87,7 +87,7 @@ func isRootCmd(command *cobra.Command) bool { return command != nil && !command.HasParent() } -func rootHelpFunc(f *cmdutil.Factory, command *cobra.Command, args []string) { +func rootHelpFunc(f *cmdutil.Factory, command *cobra.Command, _ []string) { flags := command.Flags() if isRootCmd(command) { diff --git a/pkg/cmd/root/help_topic.go b/pkg/cmd/root/help_topic.go index db0ef098d18..51fb0662a17 100644 --- a/pkg/cmd/root/help_topic.go +++ b/pkg/cmd/root/help_topic.go @@ -81,6 +81,9 @@ var HelpTopics = []helpTopic{ %[1]sCLICOLOR_FORCE%[1]s: set to a value other than %[1]s0%[1]s to keep ANSI colors in output even when the output is piped. + %[1]sGH_COLOR_LABELS%[1]s: set to any value to display labels using their RGB hex color codes in terminals that + support truecolor. + %[1]sGH_FORCE_TTY%[1]s: set to any value to force terminal-style output even when the output is redirected. When the value is a number, it is interpreted as the number of columns available in the viewport. When the value is a percentage, it will be applied against From e067eacd8114e0880c79e256eb11a119b647a9de Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Fri, 4 Apr 2025 11:57:37 -0400 Subject: [PATCH 4/6] Refactor ColorScheme initializer This commit completely removes the iostreams.NewColorScheme() initializer function in favor of exporting the type fields for greater clarity in its use. The result being code specifying only the fields that matter to test cases. --- internal/tableprinter/table_printer.go | 2 +- pkg/cmd/gist/list/list_test.go | 27 ++-- pkg/iostreams/color.go | 77 +++++------- pkg/iostreams/color_test.go | 168 +++++++++++++++++++------ pkg/iostreams/iostreams.go | 9 +- 5 files changed, 189 insertions(+), 94 deletions(-) diff --git a/internal/tableprinter/table_printer.go b/internal/tableprinter/table_printer.go index 69b22be122c..47128afb4bd 100644 --- a/internal/tableprinter/table_printer.go +++ b/internal/tableprinter/table_printer.go @@ -73,7 +73,7 @@ func NewWithWriter(w io.Writer, isTTY bool, maxWidth int, cs *iostreams.ColorSch // was not padded. In tests cs.Enabled() is false which allows us to avoid having to fix up // numerous tests that verify header padding. var paddingFunc func(int, string) string - if cs.Enabled() { + if cs.Enabled { paddingFunc = text.PadRight } diff --git a/pkg/cmd/gist/list/list_test.go b/pkg/cmd/gist/list/list_test.go index 8fbf4d6c920..14351418f51 100644 --- a/pkg/cmd/gist/list/list_test.go +++ b/pkg/cmd/gist/list/list_test.go @@ -654,50 +654,57 @@ func Test_highlightMatch(t *testing.T) { tests := []struct { name string input string - color bool + cs *iostreams.ColorScheme want string }{ { name: "single match", input: "Octo", + cs: &iostreams.ColorScheme{}, want: "Octo", }, { name: "single match (color)", input: "Octo", - color: true, - want: "\x1b[0;30;43mOcto\x1b[0m", + cs: &iostreams.ColorScheme{ + Enabled: true, + }, + want: "\x1b[0;30;43mOcto\x1b[0m", }, { name: "single match with extra", input: "Hello, Octocat!", + cs: &iostreams.ColorScheme{}, want: "Hello, Octocat!", }, { name: "single match with extra (color)", input: "Hello, Octocat!", - color: true, - want: "\x1b[0;34mHello, \x1b[0m\x1b[0;30;43mOcto\x1b[0m\x1b[0;34mcat!\x1b[0m", + cs: &iostreams.ColorScheme{ + Enabled: true, + }, + want: "\x1b[0;34mHello, \x1b[0m\x1b[0;30;43mOcto\x1b[0m\x1b[0;34mcat!\x1b[0m", }, { name: "multiple matches", input: "Octocat/octo", + cs: &iostreams.ColorScheme{}, want: "Octocat/octo", }, { name: "multiple matches (color)", input: "Octocat/octo", - color: true, - want: "\x1b[0;30;43mOcto\x1b[0m\x1b[0;34mcat/\x1b[0m\x1b[0;30;43mocto\x1b[0m", + cs: &iostreams.ColorScheme{ + Enabled: true, + }, + want: "\x1b[0;30;43mOcto\x1b[0m\x1b[0;34mcat/\x1b[0m\x1b[0;30;43mocto\x1b[0m", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - cs := iostreams.NewColorScheme(tt.color, false, false, false, false, iostreams.NoTheme) - matched := false - got, err := highlightMatch(tt.input, regex, &matched, cs.Blue, cs.Highlight) + got, err := highlightMatch(tt.input, regex, &matched, tt.cs.Blue, tt.cs.Highlight) assert.NoError(t, err) assert.True(t, matched) assert.Equal(t, tt.want, got) diff --git a/pkg/iostreams/color.go b/pkg/iostreams/color.go index 92b94c3609a..86512df4e5c 100644 --- a/pkg/iostreams/color.go +++ b/pkg/iostreams/color.go @@ -41,35 +41,24 @@ var ( } ) -// NewColorScheme initializes color logic based on provided terminal capabilities. -// Logic dealing with terminal theme detected, such as whether color is enabled, 8-bit color supported, true color supported, -// labels are colored, and terminal theme detected. -func NewColorScheme(enabled, is256enabled, trueColor, accessibleColors, colorLabels bool, theme string) *ColorScheme { - return &ColorScheme{ - enabled: enabled, - is256enabled: is256enabled, - hasTrueColor: trueColor, - accessibleColors: accessibleColors, - colorLabels: colorLabels, - theme: theme, - } -} - +// ColorScheme controls how text is colored based upon terminal capabilities and user preferences. type ColorScheme struct { - enabled bool - is256enabled bool - hasTrueColor bool - accessibleColors bool - colorLabels bool - theme string -} - -func (c *ColorScheme) Enabled() bool { - return c.enabled + // Enabled is whether color is used at all. + Enabled bool + // EightBitColor is whether the terminal supports 8-bit, 256 colors. + EightBitColor bool + // TrueColor is whether the terminal supports 24-bit, 16 million colors. + TrueColor bool + // Accessible is whether colors must be base 16 colors that users can customize in terminal preferences. + Accessible bool + // ColorLabels is whether labels are colored based on their truecolor RGB hex color. + ColorLabels bool + // Theme is the terminal background color theme used to contextually color text for light, dark, or none at all. + Theme string } func (c *ColorScheme) Bold(t string) string { - if !c.enabled { + if !c.Enabled { return t } return bold(t) @@ -81,16 +70,16 @@ func (c *ColorScheme) Boldf(t string, args ...interface{}) string { func (c *ColorScheme) Muted(t string) string { // Fallback to previous logic if accessible colors preview is disabled. - if !c.accessibleColors { + if !c.Accessible { return c.Gray(t) } // Muted text is only stylized if color is enabled. - if !c.enabled { + if !c.Enabled { return t } - switch c.theme { + switch c.Theme { case LightTheme: return lightThemeMuted(t) case DarkTheme: @@ -105,7 +94,7 @@ func (c *ColorScheme) Mutedf(t string, args ...interface{}) string { } func (c *ColorScheme) Red(t string) string { - if !c.enabled { + if !c.Enabled { return t } return red(t) @@ -116,7 +105,7 @@ func (c *ColorScheme) Redf(t string, args ...interface{}) string { } func (c *ColorScheme) Yellow(t string) string { - if !c.enabled { + if !c.Enabled { return t } return yellow(t) @@ -127,7 +116,7 @@ func (c *ColorScheme) Yellowf(t string, args ...interface{}) string { } func (c *ColorScheme) Green(t string) string { - if !c.enabled { + if !c.Enabled { return t } return green(t) @@ -138,7 +127,7 @@ func (c *ColorScheme) Greenf(t string, args ...interface{}) string { } func (c *ColorScheme) GreenBold(t string) string { - if !c.enabled { + if !c.Enabled { return t } return greenBold(t) @@ -146,10 +135,10 @@ func (c *ColorScheme) GreenBold(t string) string { // Use Muted instead for thematically contrasting color. func (c *ColorScheme) Gray(t string) string { - if !c.enabled { + if !c.Enabled { return t } - if c.is256enabled { + if c.EightBitColor { return gray256(t) } return gray(t) @@ -161,7 +150,7 @@ func (c *ColorScheme) Grayf(t string, args ...interface{}) string { } func (c *ColorScheme) Magenta(t string) string { - if !c.enabled { + if !c.Enabled { return t } return magenta(t) @@ -172,7 +161,7 @@ func (c *ColorScheme) Magentaf(t string, args ...interface{}) string { } func (c *ColorScheme) Cyan(t string) string { - if !c.enabled { + if !c.Enabled { return t } return cyan(t) @@ -183,14 +172,14 @@ func (c *ColorScheme) Cyanf(t string, args ...interface{}) string { } func (c *ColorScheme) CyanBold(t string) string { - if !c.enabled { + if !c.Enabled { return t } return cyanBold(t) } func (c *ColorScheme) Blue(t string) string { - if !c.enabled { + if !c.Enabled { return t } return blue(t) @@ -221,7 +210,7 @@ func (c *ColorScheme) FailureIconWithColor(colo func(string) string) string { } func (c *ColorScheme) HighlightStart() string { - if !c.enabled { + if !c.Enabled { return "" } @@ -229,7 +218,7 @@ func (c *ColorScheme) HighlightStart() string { } func (c *ColorScheme) Highlight(t string) string { - if !c.enabled { + if !c.Enabled { return t } @@ -237,7 +226,7 @@ func (c *ColorScheme) Highlight(t string) string { } func (c *ColorScheme) Reset() string { - if !c.enabled { + if !c.Enabled { return "" } @@ -275,7 +264,7 @@ func (c *ColorScheme) ColorFromString(s string) func(string) string { // Label stylizes text based on label's RGB hex color. func (c *ColorScheme) Label(hex string, x string) string { - if !c.enabled || !c.hasTrueColor || !c.colorLabels || len(hex) != 6 { + if !c.Enabled || !c.TrueColor || !c.ColorLabels || len(hex) != 6 { return x } @@ -287,11 +276,11 @@ func (c *ColorScheme) Label(hex string, x string) string { func (c *ColorScheme) TableHeader(t string) string { // Table headers are only stylized if color is enabled including underline modifier. - if !c.enabled { + if !c.Enabled { return t } - switch c.theme { + switch c.Theme { case DarkTheme: return darkThemeTableHeader(t) case LightTheme: diff --git a/pkg/iostreams/color_test.go b/pkg/iostreams/color_test.go index 2adacd63fed..f6a72e2a71c 100644 --- a/pkg/iostreams/color_test.go +++ b/pkg/iostreams/color_test.go @@ -20,35 +20,52 @@ func TestLabel(t *testing.T) { hex: "fc0303", text: "red", wants: "\033[38;2;252;3;3mred\033[0m", - cs: NewColorScheme(true, true, true, false, true, NoTheme), + cs: &ColorScheme{ + Enabled: true, + EightBitColor: true, + TrueColor: true, + ColorLabels: true, + }, }, { name: "no truecolor", hex: "fc0303", text: "red", wants: "red", - cs: NewColorScheme(true, true, false, false, true, NoTheme), + cs: &ColorScheme{ + Enabled: true, + EightBitColor: true, + ColorLabels: true, + }, }, { name: "no color", hex: "fc0303", text: "red", wants: "red", - cs: NewColorScheme(false, false, false, false, true, NoTheme), + cs: &ColorScheme{ + ColorLabels: true, + }, }, { name: "invalid hex", hex: "fc0", text: "red", wants: "red", - cs: NewColorScheme(false, false, false, false, true, NoTheme), + cs: &ColorScheme{ + ColorLabels: true, + }, }, { name: "no color labels", hex: "fc0303", text: "red", wants: "red", - cs: NewColorScheme(true, true, true, false, false, NoTheme), + cs: &ColorScheme{ + Enabled: true, + EightBitColor: true, + ColorLabels: true, + }, }, } @@ -71,62 +88,110 @@ func TestTableHeader(t *testing.T) { expected string }{ { - name: "when color is disabled, text is not stylized", - cs: NewColorScheme(false, false, false, true, false, NoTheme), + name: "when color is disabled, text is not stylized", + cs: &ColorScheme{ + Accessible: true, + Theme: NoTheme, + }, input: "this should not be stylized", expected: "this should not be stylized", }, { - name: "when 4-bit color is enabled but no theme, 4-bit default color and underline are used", - cs: NewColorScheme(true, false, false, true, false, NoTheme), + name: "when 4-bit color is enabled but no theme, 4-bit default color and underline are used", + cs: &ColorScheme{ + Enabled: true, + Accessible: true, + Theme: NoTheme, + }, input: "this should have no explicit color but underlined", expected: fmt.Sprintf("%sthis should have no explicit color but underlined%s", defaultUnderline, reset), }, { - name: "when 4-bit color is enabled and theme is light, 4-bit dark color and underline are used", - cs: NewColorScheme(true, false, false, true, false, LightTheme), + name: "when 4-bit color is enabled and theme is light, 4-bit dark color and underline are used", + cs: &ColorScheme{ + Enabled: true, + Accessible: true, + Theme: LightTheme, + }, input: "this should have dark foreground color and underlined", expected: fmt.Sprintf("%sthis should have dark foreground color and underlined%s", brightBlackUnderline, reset), }, { - name: "when 4-bit color is enabled and theme is dark, 4-bit light color and underline are used", - cs: NewColorScheme(true, false, false, true, false, DarkTheme), + name: "when 4-bit color is enabled and theme is dark, 4-bit light color and underline are used", + cs: &ColorScheme{ + Enabled: true, + Accessible: true, + Theme: DarkTheme, + }, input: "this should have light foreground color and underlined", expected: fmt.Sprintf("%sthis should have light foreground color and underlined%s", dimBlackUnderline, reset), }, { - name: "when 8-bit color is enabled but no theme, 4-bit default color and underline are used", - cs: NewColorScheme(true, true, false, true, false, NoTheme), + name: "when 8-bit color is enabled but no theme, 4-bit default color and underline are used", + cs: &ColorScheme{ + Enabled: true, + EightBitColor: true, + Accessible: true, + Theme: NoTheme, + }, input: "this should have no explicit color but underlined", expected: fmt.Sprintf("%sthis should have no explicit color but underlined%s", defaultUnderline, reset), }, { - name: "when 8-bit color is enabled and theme is light, 4-bit dark color and underline are used", - cs: NewColorScheme(true, true, false, true, false, LightTheme), + name: "when 8-bit color is enabled and theme is light, 4-bit dark color and underline are used", + cs: &ColorScheme{ + Enabled: true, + EightBitColor: true, + Accessible: true, + Theme: LightTheme, + }, input: "this should have dark foreground color and underlined", expected: fmt.Sprintf("%sthis should have dark foreground color and underlined%s", brightBlackUnderline, reset), }, { - name: "when 8-bit color is true and theme is dark, 4-bit light color and underline are used", - cs: NewColorScheme(true, true, false, true, false, DarkTheme), + name: "when 8-bit color is true and theme is dark, 4-bit light color and underline are used", + cs: &ColorScheme{ + Enabled: true, + EightBitColor: true, + Accessible: true, + Theme: DarkTheme, + }, input: "this should have light foreground color and underlined", expected: fmt.Sprintf("%sthis should have light foreground color and underlined%s", dimBlackUnderline, reset), }, { - name: "when 24-bit color is enabled but no theme, 4-bit default color and underline are used", - cs: NewColorScheme(true, true, true, true, false, NoTheme), + name: "when 24-bit color is enabled but no theme, 4-bit default color and underline are used", + cs: &ColorScheme{ + Enabled: true, + EightBitColor: true, + TrueColor: true, + Accessible: true, + Theme: NoTheme, + }, input: "this should have no explicit color but underlined", expected: fmt.Sprintf("%sthis should have no explicit color but underlined%s", defaultUnderline, reset), }, { - name: "when 24-bit color is enabled and theme is light, 4-bit dark color and underline are used", - cs: NewColorScheme(true, true, true, true, false, LightTheme), + name: "when 24-bit color is enabled and theme is light, 4-bit dark color and underline are used", + cs: &ColorScheme{ + Enabled: true, + EightBitColor: true, + TrueColor: true, + Accessible: true, + Theme: LightTheme, + }, input: "this should have dark foreground color and underlined", expected: fmt.Sprintf("%sthis should have dark foreground color and underlined%s", brightBlackUnderline, reset), }, { - name: "when 24-bit color is true and theme is dark, 4-bit light color and underline are used", - cs: NewColorScheme(true, true, true, true, false, DarkTheme), + name: "when 24-bit color is true and theme is dark, 4-bit light color and underline are used", + cs: &ColorScheme{ + Enabled: true, + EightBitColor: true, + TrueColor: true, + Accessible: true, + Theme: DarkTheme, + }, input: "this should have light foreground color and underlined", expected: fmt.Sprintf("%sthis should have light foreground color and underlined%s", dimBlackUnderline, reset), }, @@ -154,43 +219,70 @@ func TestMuted(t *testing.T) { }{ { name: "when color is disabled but accessible colors are disabled, text is not stylized", - cs: NewColorScheme(false, false, false, false, false, NoTheme), + cs: &ColorScheme{}, input: "this should not be stylized", expected: "this should not be stylized", }, { - name: "when 4-bit color is enabled but accessible colors are disabled, legacy 4-bit gray color is used", - cs: NewColorScheme(true, false, false, false, false, NoTheme), + name: "when 4-bit color is enabled but accessible colors are disabled, legacy 4-bit gray color is used", + cs: &ColorScheme{ + Enabled: true, + }, input: "this should be 4-bit gray", expected: fmt.Sprintf("%sthis should be 4-bit gray%s", gray4bit, reset), }, { - name: "when 8-bit color is enabled but accessible colors are disabled, legacy 8-bit gray color is used", - cs: NewColorScheme(true, true, false, false, false, NoTheme), + name: "when 8-bit color is enabled but accessible colors are disabled, legacy 8-bit gray color is used", + cs: &ColorScheme{ + Enabled: true, + EightBitColor: true, + }, input: "this should be 8-bit gray", expected: fmt.Sprintf("%sthis should be 8-bit gray%s", gray8bit, reset), }, { - name: "when 24-bit color is enabled but accessible colors are disabled, legacy 8-bit gray color is used", - cs: NewColorScheme(true, true, true, false, false, NoTheme), + name: "when 24-bit color is enabled but accessible colors are disabled, legacy 8-bit gray color is used", + cs: &ColorScheme{ + Enabled: true, + EightBitColor: true, + TrueColor: true, + }, input: "this should be 8-bit gray", expected: fmt.Sprintf("%sthis should be 8-bit gray%s", gray8bit, reset), }, { - name: "when 4-bit color is enabled and theme is dark, 4-bit light color is used", - cs: NewColorScheme(true, true, true, true, false, DarkTheme), + name: "when 4-bit color is enabled and theme is dark, 4-bit light color is used", + cs: &ColorScheme{ + Enabled: true, + EightBitColor: true, + TrueColor: true, + Accessible: true, + Theme: DarkTheme, + }, input: "this should be 4-bit dim black", expected: fmt.Sprintf("%sthis should be 4-bit dim black%s", dimBlack4bit, reset), }, { - name: "when 4-bit color is enabled and theme is light, 4-bit dark color is used", - cs: NewColorScheme(true, true, true, true, false, LightTheme), + name: "when 4-bit color is enabled and theme is light, 4-bit dark color is used", + cs: &ColorScheme{ + Enabled: true, + EightBitColor: true, + TrueColor: true, + Accessible: true, + Theme: LightTheme, + }, input: "this should be 4-bit bright black", expected: fmt.Sprintf("%sthis should be 4-bit bright black%s", brightBlack4bit, reset), }, { - name: "when 4-bit color is enabled but no theme, 4-bit default color is used", - cs: NewColorScheme(true, true, true, true, false, NoTheme), + name: "when 4-bit color is enabled but no theme, 4-bit default color is used", + cs: &ColorScheme{ + Enabled: true, + EightBitColor: true, + TrueColor: true, + Accessible: true, + Theme: NoTheme, + }, input: "this should have no explicit color", expected: "this should have no explicit color", }, diff --git a/pkg/iostreams/iostreams.go b/pkg/iostreams/iostreams.go index 28e81abbaa5..f5e3c2aee39 100644 --- a/pkg/iostreams/iostreams.go +++ b/pkg/iostreams/iostreams.go @@ -376,7 +376,14 @@ func (s *IOStreams) TerminalWidth() int { } func (s *IOStreams) ColorScheme() *ColorScheme { - return NewColorScheme(s.ColorEnabled(), s.ColorSupport256(), s.HasTrueColor(), s.AccessibleColorsEnabled(), s.ColorLabels(), s.TerminalTheme()) + return &ColorScheme{ + Enabled: s.ColorEnabled(), + EightBitColor: s.ColorSupport256(), + TrueColor: s.HasTrueColor(), + Accessible: s.AccessibleColorsEnabled(), + ColorLabels: s.ColorLabels(), + Theme: s.TerminalTheme(), + } } func (s *IOStreams) ReadUserFile(fn string) ([]byte, error) { From c5206109ca9db2d2215b1de68c5ccc0cb9bfa548 Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Tue, 8 Apr 2025 15:24:18 -0400 Subject: [PATCH 5/6] Use truthy value for `GH_COLOR_LABELS` --- pkg/cmd/factory/default.go | 9 +++++++-- pkg/cmd/factory/default_test.go | 32 +++++++++++++++++++++++++++++++- 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/pkg/cmd/factory/default.go b/pkg/cmd/factory/default.go index 68df7a3169d..6286c999d0a 100644 --- a/pkg/cmd/factory/default.go +++ b/pkg/cmd/factory/default.go @@ -293,8 +293,13 @@ func ioStreams(f *cmdutil.Factory) *iostreams.IOStreams { io.SetPager(pager.Value) } - if _, ghColorLabels := os.LookupEnv("GH_COLOR_LABELS"); ghColorLabels { - io.SetColorLabels(true) // TODO: should this be a truthy value? + if ghColorLabels, ghColorLabelsExists := os.LookupEnv("GH_COLOR_LABELS"); ghColorLabelsExists { + switch ghColorLabels { + case "", "0", "false", "no": + io.SetColorLabels(false) + default: + io.SetColorLabels(true) + } } else if prompt := cfg.ColorLabels(""); prompt.Value == "enabled" { io.SetColorLabels(true) } diff --git a/pkg/cmd/factory/default_test.go b/pkg/cmd/factory/default_test.go index b1730d6e615..407c2fcdb15 100644 --- a/pkg/cmd/factory/default_test.go +++ b/pkg/cmd/factory/default_test.go @@ -449,10 +449,40 @@ func Test_ioStreams_colorLabels(t *testing.T) { colorLabelsEnabled: true, }, { - name: "colorLabels enabled via GH_COLOR_LABELS env var", + name: "colorLabels enabled via `1` in GH_COLOR_LABELS env var", env: map[string]string{"GH_COLOR_LABELS": "1"}, colorLabelsEnabled: true, }, + { + name: "colorLabels enabled via `true` in GH_COLOR_LABELS env var", + env: map[string]string{"GH_COLOR_LABELS": "true"}, + colorLabelsEnabled: true, + }, + { + name: "colorLabels enabled via `yes` in GH_COLOR_LABELS env var", + env: map[string]string{"GH_COLOR_LABELS": "yes"}, + colorLabelsEnabled: true, + }, + { + name: "colorLabels disable via empty string in GH_COLOR_LABELS env var", + env: map[string]string{"GH_COLOR_LABELS": ""}, + colorLabelsEnabled: false, + }, + { + name: "colorLabels disabled via `0` in GH_COLOR_LABELS env var", + env: map[string]string{"GH_COLOR_LABELS": "0"}, + colorLabelsEnabled: false, + }, + { + name: "colorLabels disabled via `false` in GH_COLOR_LABELS env var", + env: map[string]string{"GH_COLOR_LABELS": "false"}, + colorLabelsEnabled: false, + }, + { + name: "colorLabels disabled via `no` in GH_COLOR_LABELS env var", + env: map[string]string{"GH_COLOR_LABELS": "no"}, + colorLabelsEnabled: false, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From 5c12f5633e3beee887be089a82b0ec52469ad087 Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Tue, 8 Apr 2025 15:27:36 -0400 Subject: [PATCH 6/6] Test for explicit config disable of label colors --- pkg/cmd/factory/default_test.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/pkg/cmd/factory/default_test.go b/pkg/cmd/factory/default_test.go index 407c2fcdb15..c0275d1dec4 100644 --- a/pkg/cmd/factory/default_test.go +++ b/pkg/cmd/factory/default_test.go @@ -448,6 +448,11 @@ func Test_ioStreams_colorLabels(t *testing.T) { config: enableColorLabelsConfig(), colorLabelsEnabled: true, }, + { + name: "config with colorLabels disabled", + config: disableColorLabelsConfig(), + colorLabelsEnabled: false, + }, { name: "colorLabels enabled via `1` in GH_COLOR_LABELS env var", env: map[string]string{"GH_COLOR_LABELS": "1"}, @@ -611,6 +616,10 @@ func disablePromptConfig() gh.Config { return config.NewFromString("prompt: disabled") } +func disableColorLabelsConfig() gh.Config { + return config.NewFromString("color_labels: disabled") +} + func enableColorLabelsConfig() gh.Config { return config.NewFromString("color_labels: enabled") }