Merge pull request #2811 from cli/utils-spinner-buh-bye

Retire utils.Spinner in favor of IOStreams.StartProgressIndicator

pkg/cmd/issue/view/view_test.go

@@ -8,7 +8,6 @@ import (
 	"os/exec"
 	"testing"
 
-	"github.com/briandowns/spinner"
 	"github.com/cli/cli/internal/config"
 	"github.com/cli/cli/internal/ghrepo"
 	"github.com/cli/cli/internal/run"
@@ -16,7 +15,6 @@ import (
 	"github.com/cli/cli/pkg/httpmock"
 	"github.com/cli/cli/pkg/iostreams"
 	"github.com/cli/cli/test"
-	"github.com/cli/cli/utils"
 	"github.com/google/shlex"
 	"github.com/stretchr/testify/assert"
 )
@@ -404,7 +402,6 @@ func TestIssueView_tty_Comments(t *testing.T) {
 	}
 	for name, tc := range tests {
 		t.Run(name, func(t *testing.T) {
-			stubSpinner()
 			http := &httpmock.Registry{}
 			defer http.Verify(t)
 			for name, file := range tc.fixtures {
@@ -496,8 +493,3 @@ func TestIssueView_nontty_Comments(t *testing.T) {
 		})
 	}
 }
-
-func stubSpinner() {
-	utils.StartSpinner = func(_ *spinner.Spinner) {}
-	utils.StopSpinner = func(_ *spinner.Spinner) {}
-}

pkg/cmd/pr/view/view_test.go

@@ -9,7 +9,6 @@ import (
 	"strings"
 	"testing"
 
-	"github.com/briandowns/spinner"
 	"github.com/cli/cli/context"
 	"github.com/cli/cli/git"
 	"github.com/cli/cli/internal/config"
@@ -19,7 +18,6 @@ import (
 	"github.com/cli/cli/pkg/httpmock"
 	"github.com/cli/cli/pkg/iostreams"
 	"github.com/cli/cli/test"
-	"github.com/cli/cli/utils"
 	"github.com/google/shlex"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -864,7 +862,6 @@ func TestPRView_tty_Comments(t *testing.T) {
 	}
 	for name, tt := range tests {
 		t.Run(name, func(t *testing.T) {
-			stubSpinner()
 			http := &httpmock.Registry{}
 			defer http.Verify(t)
 			for name, file := range tt.fixtures {
@@ -987,8 +984,3 @@ func TestPRView_nontty_Comments(t *testing.T) {
 		})
 	}
 }
-
-func stubSpinner() {
-	utils.StartSpinner = func(_ *spinner.Spinner) {}
-	utils.StopSpinner = func(_ *spinner.Spinner) {}
-}

pkg/cmd/repo/fork/fork.go

@@ -128,18 +128,6 @@ func forkRun(opts *ForkOptions) error {
 
 	cs := opts.IO.ColorScheme()
 	stderr := opts.IO.ErrOut
-	s := utils.Spinner(stderr)
-	stopSpinner := func() {}
-
-	if connectedToTerminal {
-		loading := cs.Gray("Forking ") + cs.Bold(cs.Gray(ghrepo.FullName(repoToFork))) + cs.Gray("...")
-		s.Suffix = " " + loading
-		s.FinalMSG = cs.Gray(fmt.Sprintf("- %s\n", loading))
-		utils.StartSpinner(s)
-		stopSpinner = func() {
-			utils.StopSpinner(s)
-		}
-	}
 
 	httpClient, err := opts.HttpClient()
 	if err != nil {
@@ -148,14 +136,13 @@ func forkRun(opts *ForkOptions) error {
 
 	apiClient := api.NewClientFromHTTP(httpClient)
 
+	opts.IO.StartProgressIndicator()
 	forkedRepo, err := api.ForkRepo(apiClient, repoToFork)
+	opts.IO.StopProgressIndicator()
 	if err != nil {
-		stopSpinner()
 		return fmt.Errorf("failed to fork: %w", err)
 	}
 
-	stopSpinner()
-
 	// This is weird. There is not an efficient way to determine via the GitHub API whether or not a
 	// given user has forked a given repo. We noticed, also, that the create fork API endpoint just
 	// returns the fork repo data even if it already exists -- with no change in status code or

pkg/cmd/repo/fork/fork_test.go

@@ -9,7 +9,6 @@ import (
 	"testing"
 	"time"
 
-	"github.com/briandowns/spinner"
 	"github.com/cli/cli/context"
 	"github.com/cli/cli/git"
 	"github.com/cli/cli/internal/config"
@@ -20,7 +19,6 @@ import (
 	"github.com/cli/cli/pkg/iostreams"
 	"github.com/cli/cli/pkg/prompt"
 	"github.com/cli/cli/test"
-	"github.com/cli/cli/utils"
 	"github.com/google/shlex"
 	"github.com/stretchr/testify/assert"
 )
@@ -204,7 +202,6 @@ func TestRepoFork_outside_parent_nontty(t *testing.T) {
 }
 
 func TestRepoFork_already_forked(t *testing.T) {
-	stubSpinner()
 	reg := &httpmock.Registry{}
 	defer reg.StubWithFixturePath(200, "./forkResult.json")()
 	httpClient := &http.Client{Transport: reg}
@@ -229,7 +226,6 @@ func TestRepoFork_already_forked(t *testing.T) {
 }
 
 func TestRepoFork_reuseRemote(t *testing.T) {
-	stubSpinner()
 	remotes := []*context.Remote{
 		{
 			Remote: &git.Remote{Name: "origin", FetchURL: &url.URL{}},
@@ -257,7 +253,6 @@ func TestRepoFork_reuseRemote(t *testing.T) {
 }
 
 func TestRepoFork_in_parent(t *testing.T) {
-	stubSpinner()
 	reg := &httpmock.Registry{}
 	defer reg.StubWithFixturePath(200, "./forkResult.json")()
 	httpClient := &http.Client{Transport: reg}
@@ -283,7 +278,6 @@ func TestRepoFork_in_parent(t *testing.T) {
 }
 
 func TestRepoFork_outside(t *testing.T) {
-	stubSpinner()
 	tests := []struct {
 		name string
 		args string
@@ -322,7 +316,6 @@ func TestRepoFork_outside(t *testing.T) {
 }
 
 func TestRepoFork_in_parent_yes(t *testing.T) {
-	stubSpinner()
 	defer stubSince(2 * time.Second)()
 	reg := &httpmock.Registry{}
 	defer reg.StubWithFixturePath(200, "./forkResult.json")()
@@ -352,7 +345,6 @@ func TestRepoFork_in_parent_yes(t *testing.T) {
 }
 
 func TestRepoFork_outside_yes(t *testing.T) {
-	stubSpinner()
 	defer stubSince(2 * time.Second)()
 	reg := &httpmock.Registry{}
 	defer reg.StubWithFixturePath(200, "./forkResult.json")()
@@ -381,7 +373,6 @@ func TestRepoFork_outside_yes(t *testing.T) {
 }
 
 func TestRepoFork_outside_survey_yes(t *testing.T) {
-	stubSpinner()
 	defer stubSince(2 * time.Second)()
 	reg := &httpmock.Registry{}
 	defer reg.StubWithFixturePath(200, "./forkResult.json")()
@@ -412,7 +403,6 @@ func TestRepoFork_outside_survey_yes(t *testing.T) {
 }
 
 func TestRepoFork_outside_survey_no(t *testing.T) {
-	stubSpinner()
 	defer stubSince(2 * time.Second)()
 	reg := &httpmock.Registry{}
 	defer reg.StubWithFixturePath(200, "./forkResult.json")()
@@ -444,7 +434,6 @@ func TestRepoFork_outside_survey_no(t *testing.T) {
 }
 
 func TestRepoFork_in_parent_survey_yes(t *testing.T) {
-	stubSpinner()
 	reg := &httpmock.Registry{}
 	defer reg.StubWithFixturePath(200, "./forkResult.json")()
 	httpClient := &http.Client{Transport: reg}
@@ -476,7 +465,6 @@ func TestRepoFork_in_parent_survey_yes(t *testing.T) {
 }
 
 func TestRepoFork_in_parent_survey_no(t *testing.T) {
-	stubSpinner()
 	reg := &httpmock.Registry{}
 	defer reg.StubWithFixturePath(200, "./forkResult.json")()
 	httpClient := &http.Client{Transport: reg}
@@ -508,7 +496,6 @@ func TestRepoFork_in_parent_survey_no(t *testing.T) {
 }
 
 func TestRepoFork_in_parent_match_protocol(t *testing.T) {
-	stubSpinner()
 	defer stubSince(2 * time.Second)()
 	reg := &httpmock.Registry{}
 	defer reg.StubWithFixturePath(200, "./forkResult.json")()
@@ -546,14 +533,6 @@ func TestRepoFork_in_parent_match_protocol(t *testing.T) {
 	reg.Verify(t)
 }
 
-func stubSpinner() {
-	// not bothering with teardown since we never want spinners when doing tests
-	utils.StartSpinner = func(_ *spinner.Spinner) {
-	}
-	utils.StopSpinner = func(_ *spinner.Spinner) {
-	}
-}
-
 func stubSince(d time.Duration) func() {
 	originalSince := Since
 	Since = func(t time.Time) time.Duration {

utils/utils.go

@@ -2,12 +2,10 @@ package utils
 
 import (
 	"fmt"
-	"io"
 	"net/url"
 	"strings"
 	"time"
 
-	"github.com/briandowns/spinner"
 	"github.com/cli/cli/internal/run"
 	"github.com/cli/cli/pkg/browser"
 )
@@ -88,20 +86,6 @@ func Humanize(s string) string {
 	return strings.Map(h, s)
 }
 
-// We do this so we can stub out the spinner in tests -- it made things really flakey. This is not
-// an elegant solution.
-var StartSpinner = func(s *spinner.Spinner) {
-	s.Start()
-}
-
-var StopSpinner = func(s *spinner.Spinner) {
-	s.Stop()
-}
-
-func Spinner(w io.Writer) *spinner.Spinner {
-	return spinner.New(spinner.CharSets[11], 400*time.Millisecond, spinner.WithWriter(w))
-}
-
 func IsURL(s string) bool {
 	return strings.HasPrefix(s, "http:/") || strings.HasPrefix(s, "https:/")
 }