From 3eba461afb2746576cd26ed391683494c4f48dd5 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Mon, 14 Apr 2025 15:07:11 -0600 Subject: [PATCH] fix(iostreams): spinner env var is now in factory - Move env var evaluation to the factory defaults. - Use only the label for the spinner instead of adding "working..." every time. "working..." remains the default when no label is provided. --- pkg/cmd/factory/default.go | 7 +++ pkg/cmd/factory/default_test.go | 50 +++++++++++++++++++ pkg/iostreams/iostreams.go | 33 ++++++++---- .../iostreams_progress_indicator_test.go | 18 +++---- 4 files changed, 88 insertions(+), 20 deletions(-) diff --git a/pkg/cmd/factory/default.go b/pkg/cmd/factory/default.go index 6286c999d..7a45efeb4 100644 --- a/pkg/cmd/factory/default.go +++ b/pkg/cmd/factory/default.go @@ -6,6 +6,7 @@ import ( "net/http" "os" "regexp" + "slices" "time" "github.com/cli/cli/v2/api" @@ -283,6 +284,12 @@ func ioStreams(f *cmdutil.Factory) *iostreams.IOStreams { io.SetNeverPrompt(true) } + ghSpinnerDisabledValue, ghSpinnerDisabledIsSet := os.LookupEnv("GH_SPINNER_DISABLED") + falseyValues := []string{"false", "0", "no", ""} + if ghSpinnerDisabledIsSet && !slices.Contains(falseyValues, ghSpinnerDisabledValue) { + io.SetSpinnerDisabled(true) + } + // Pager precedence // 1. GH_PAGER // 2. pager from config diff --git a/pkg/cmd/factory/default_test.go b/pkg/cmd/factory/default_test.go index c0275d1de..aff833d4c 100644 --- a/pkg/cmd/factory/default_test.go +++ b/pkg/cmd/factory/default_test.go @@ -432,6 +432,56 @@ func Test_ioStreams_prompt(t *testing.T) { } } +func Test_ioStreams_spinnerDisabled(t *testing.T) { + tests := []struct { + name string + spinnerDisabled bool + env map[string]string + }{ + { + name: "default config", + spinnerDisabled: false, + }, + { + name: "spinner disabled via GH_SPINNER_DISABLED env var = 0", + env: map[string]string{"GH_SPINNER_DISABLED": "0"}, + spinnerDisabled: false, + }, + { + name: "spinner disabled via GH_SPINNER_DISABLED env var = false", + env: map[string]string{"GH_SPINNER_DISABLED": "false"}, + spinnerDisabled: false, + }, + { + name: "spinner disabled via GH_SPINNER_DISABLED env var = no", + env: map[string]string{"GH_SPINNER_DISABLED": "no"}, + spinnerDisabled: false, + }, + { + name: "spinner enabled via GH_SPINNER_DISABLED env var = 1", + env: map[string]string{"GH_SPINNER_DISABLED": "1"}, + spinnerDisabled: true, + }, + { + name: "spinner enabled via GH_SPINNER_DISABLED env var = true", + env: map[string]string{"GH_SPINNER_DISABLED": "true"}, + spinnerDisabled: 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") + io := ioStreams(f) + assert.Equal(t, tt.spinnerDisabled, io.GetSpinnerDisabled()) + }) + } +} + func Test_ioStreams_colorLabels(t *testing.T) { tests := []struct { name string diff --git a/pkg/iostreams/iostreams.go b/pkg/iostreams/iostreams.go index 8608978c5..07d7aa27f 100644 --- a/pkg/iostreams/iostreams.go +++ b/pkg/iostreams/iostreams.go @@ -8,7 +8,6 @@ import ( "os" "os/exec" "os/signal" - "slices" "strings" "sync" "time" @@ -79,7 +78,8 @@ type IOStreams struct { pagerCommand string pagerProcess *os.Process - neverPrompt bool + neverPrompt bool + spinnerDisabled bool TempFileOverride *os.File } @@ -274,6 +274,14 @@ func (s *IOStreams) SetNeverPrompt(v bool) { s.neverPrompt = v } +func (s *IOStreams) GetSpinnerDisabled() bool { + return s.spinnerDisabled +} + +func (s *IOStreams) SetSpinnerDisabled(v bool) { + s.spinnerDisabled = v +} + func (s *IOStreams) StartProgressIndicator() { s.StartProgressIndicatorWithLabel("") } @@ -295,13 +303,20 @@ func (s *IOStreams) StartProgressIndicatorWithLabel(label string) { return } - spinnerDisabledValue, spinnerDisabledIsSet := os.LookupEnv("GH_SPINNER_DISABLED") - falseyValues := []string{"false", "0", "no", ""} - var spinnerStyle []string - if spinnerDisabledIsSet && !slices.Contains(falseyValues, spinnerDisabledValue) { - // This progress indicator must be readable by screen readers - spinnerStyle = []string{"Working..."} + if s.spinnerDisabled { + // Default label when spinner disabled is "Working..." + if label == "" { + label = "Working..." + } + + // Add an ellipsis to the label if it doesn't already have one. + ellipsis := "..." + if !strings.HasSuffix(label, ellipsis) { + label = label + ellipsis + } + + spinnerStyle = []string{label} } else { // https://github.com/briandowns/spinner#available-character-sets // ⣾ ⣷ ⣽ ⣻ ⡿ @@ -309,7 +324,7 @@ func (s *IOStreams) StartProgressIndicatorWithLabel(label string) { } sp := spinner.New(spinnerStyle, 120*time.Millisecond, spinner.WithWriter(s.ErrOut), spinner.WithColor("fgCyan")) - if label != "" { + if label != "" && !s.spinnerDisabled { sp.Prefix = label + " " } diff --git a/pkg/iostreams/iostreams_progress_indicator_test.go b/pkg/iostreams/iostreams_progress_indicator_test.go index c7c374811..3cd6eaf65 100644 --- a/pkg/iostreams/iostreams_progress_indicator_test.go +++ b/pkg/iostreams/iostreams_progress_indicator_test.go @@ -34,8 +34,7 @@ func TestStartProgressIndicatorWithLabel(t *testing.T) { // from the console. t.Run("progress indicator respects GH_SPINNER_DISABLED is true", func(t *testing.T) { console := newTestVirtualTerminal(t) - io := newTestIOStreams(t, console) - t.Setenv("GH_SPINNER_DISABLED", "true") + io := newTestIOStreams(t, console, true) done := make(chan error) @@ -57,8 +56,7 @@ func TestStartProgressIndicatorWithLabel(t *testing.T) { t.Run("progress indicator respects GH_SPINNER_DISABLED is false", func(t *testing.T) { console := newTestVirtualTerminal(t) - io := newTestIOStreams(t, console) - t.Setenv("GH_SPINNER_DISABLED", "false") + io := newTestIOStreams(t, console, false) done := make(chan error) @@ -80,16 +78,13 @@ func TestStartProgressIndicatorWithLabel(t *testing.T) { t.Run("progress indicator with GH_SPINNER_DISABLED shows label", func(t *testing.T) { console := newTestVirtualTerminal(t) - io := newTestIOStreams(t, console) - t.Setenv("GH_SPINNER_DISABLED", "true") + io := newTestIOStreams(t, console, true) progressIndicatorLabel := "downloading happiness" done := make(chan error) go func() { - _, err := console.ExpectString("Working...") - require.NoError(t, err) - _, err = console.ExpectString(progressIndicatorLabel) + _, err := console.ExpectString(progressIndicatorLabel + "...") done <- err }() @@ -106,7 +101,7 @@ func TestStartProgressIndicatorWithLabel(t *testing.T) { t.Run("progress indicator shows label and spinner", func(t *testing.T) { console := newTestVirtualTerminal(t) - io := newTestIOStreams(t, console) + io := newTestIOStreams(t, console, false) progressIndicatorLabel := "downloading happiness" done := make(chan error) @@ -156,7 +151,7 @@ func newTestVirtualTerminal(t *testing.T) *expect.Console { return console } -func newTestIOStreams(t *testing.T, console *expect.Console) *IOStreams { +func newTestIOStreams(t *testing.T, console *expect.Console, spinnerDisabled bool) *IOStreams { t.Helper() in := console.Tty() @@ -174,6 +169,7 @@ func newTestIOStreams(t *testing.T, console *expect.Console) *IOStreams { term: fakeTerm{}, } io.progressIndicatorEnabled = true + io.SetSpinnerDisabled(spinnerDisabled) return io }