From 9b86fe41c07bcaf20987ad968ff0e09ba670d729 Mon Sep 17 00:00:00 2001 From: Greggory Rothmeier Date: Tue, 21 Jun 2022 06:47:11 -0700 Subject: [PATCH] Simplify codespace picker (#5826) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This doesn't change anything about how it's formatted, but extracts a new function formatCodespacesForSelect so we can test and see how it treats the different combinations of codespaces. Co-authored-by: Mislav Marohnić --- pkg/cmd/codespace/common.go | 123 ++++--------------- pkg/cmd/codespace/common_test.go | 195 ++++++++++++++++++++++++++----- 2 files changed, 186 insertions(+), 132 deletions(-) diff --git a/pkg/cmd/codespace/common.go b/pkg/cmd/codespace/common.go index 6d272bcbb..16b3f7ce6 100644 --- a/pkg/cmd/codespace/common.go +++ b/pkg/cmd/codespace/common.go @@ -10,7 +10,6 @@ import ( "log" "os" "sort" - "strings" "github.com/AlecAivazis/survey/v2" "github.com/AlecAivazis/survey/v2/terminal" @@ -133,111 +132,48 @@ func chooseCodespace(ctx context.Context, apiClient apiClient) (*api.Codespace, return chooseCodespaceFromList(ctx, codespaces) } -// chooseCodespaceFromList returns the selected codespace from the list, -// or an error if there are no codespaces. +// chooseCodespaceFromList returns the codespace that the user has interactively selected from the list, or +// an error if there are no codespaces. func chooseCodespaceFromList(ctx context.Context, codespaces []*api.Codespace) (*api.Codespace, error) { if len(codespaces) == 0 { return nil, errNoCodespaces } - sort.Slice(codespaces, func(i, j int) bool { - return codespaces[i].CreatedAt > codespaces[j].CreatedAt + sortedCodespaces := codespaces + sort.Slice(sortedCodespaces, func(i, j int) bool { + return sortedCodespaces[i].CreatedAt > sortedCodespaces[j].CreatedAt }) - type codespaceWithIndex struct { - cs codespace - idx int - } - - namesWithConflict := make(map[string]bool) - codespacesByName := make(map[string]codespaceWithIndex) - codespacesNames := make([]string, 0, len(codespaces)) - codespacesDirty := make(map[string]bool) - for _, apiCodespace := range codespaces { - cs := codespace{apiCodespace} - csName := cs.displayName(false, false) - displayNameWithGitStatus := cs.displayName(false, true) - - _, hasExistingConflict := namesWithConflict[csName] - if seenCodespace, ok := codespacesByName[csName]; ok || hasExistingConflict { - // There is an existing codespace on the repo and branch. - // We need to disambiguate by adding the codespace name - // to the existing entry and the one we are processing now. - if !hasExistingConflict { - fullDisplayName := seenCodespace.cs.displayName(true, false) - fullDisplayNameWithGitStatus := seenCodespace.cs.displayName(true, true) - - codespacesByName[fullDisplayName] = codespaceWithIndex{seenCodespace.cs, seenCodespace.idx} - codespacesNames[seenCodespace.idx] = fullDisplayNameWithGitStatus - - // Update the git status dirty map to reflect the new name. - if seenCodespace.cs.hasUnsavedChanges() { - codespacesDirty[fullDisplayNameWithGitStatus] = true - } - - // delete the existing map entry with old name - delete(codespacesByName, csName) - - // All other codespaces with the same name should update - // to their specific name, this tracks conflicting names going forward - namesWithConflict[csName] = true - } - - // update this codespace names to include the name to disambiguate - csName = cs.displayName(true, false) - displayNameWithGitStatus = cs.displayName(true, true) - } - - codespacesByName[csName] = codespaceWithIndex{cs, len(codespacesNames)} - codespacesNames = append(codespacesNames, displayNameWithGitStatus) - - if cs.hasUnsavedChanges() { - codespacesDirty[displayNameWithGitStatus] = true - } - } - csSurvey := []*survey.Question{ { Name: "codespace", Prompt: &survey.Select{ Message: "Choose codespace:", - Options: codespacesNames, - Default: codespacesNames[0], + Options: formatCodespacesForSelect(sortedCodespaces), }, Validate: survey.Required, }, } var answers struct { - Codespace string + Codespace int } if err := ask(csSurvey, &answers); err != nil { return nil, fmt.Errorf("error getting answers: %w", err) } - // Codespaces are indexed without the git status included as compared - // to how it is displayed in the prompt, so the git status symbol needs - // cleaning up in case it is included. - selectedCodespace := answers.Codespace - isDirty := codespacesDirty[selectedCodespace] - if isDirty { - selectedCodespace = withoutGitStatus(answers.Codespace) - } - return codespacesByName[selectedCodespace].cs.Codespace, nil + return sortedCodespaces[answers.Codespace], nil } -// withoutGitStatus returns the string without the git status symbol. -func withoutGitStatus(cname string) string { - return replaceLastOccurence(cname, gitStatusDirty, "") -} +func formatCodespacesForSelect(codespaces []*api.Codespace) []string { + names := make([]string, len(codespaces)) -// replaceLastOccurence replaces the last occurence of a string with another string. -func replaceLastOccurence(str, old, replace string) string { - i := strings.LastIndex(str, old) - if i == -1 { - return str + for i, apiCodespace := range codespaces { + cs := codespace{apiCodespace} + names[i] = cs.displayName() } - return str[:i] + replace + str[i+len(old):] + + return names } // getOrChooseCodespace prompts the user to choose a codespace if the codespaceName is empty. @@ -339,29 +275,14 @@ type codespace struct { *api.Codespace } -// displayName returns the repository nwo and branch. -// If includeName is true, the name of the codespace (including displayName) is included. -// If includeGitStatus is true, the branch will include a star if -// the codespace has unsaved changes. -func (c codespace) displayName(includeName, includeGitStatus bool) string { - branch := c.GitStatus.Ref - if includeGitStatus { - branch = c.branchWithGitStatus() +// displayName formats the codespace name for the interactive selector prompt. +func (c codespace) displayName() string { + branch := c.branchWithGitStatus() + displayName := c.DisplayName + if displayName == "" { + displayName = c.Name } - - if includeName { - var displayName = c.Name - if c.DisplayName != "" { - displayName = c.DisplayName - } - return fmt.Sprintf( - "%s: %s (%s)", c.Repository.FullName, displayName, branch, - ) - } - return fmt.Sprintf( - "%s: %s", c.Repository.FullName, branch, - ) - + return fmt.Sprintf("%s (%s): %s", c.Repository.FullName, branch, displayName) } // gitStatusDirty represents an unsaved changes status. diff --git a/pkg/cmd/codespace/common_test.go b/pkg/cmd/codespace/common_test.go index c5fc01590..58e56ed7d 100644 --- a/pkg/cmd/codespace/common_test.go +++ b/pkg/cmd/codespace/common_test.go @@ -1,6 +1,7 @@ package codespace import ( + "reflect" "testing" "github.com/cli/cli/v2/internal/codespaces/api" @@ -10,14 +11,9 @@ func Test_codespace_displayName(t *testing.T) { type fields struct { Codespace *api.Codespace } - type args struct { - includeName bool - includeGitStatus bool - } tests := []struct { name string fields fields - args args want string }{ { @@ -33,11 +29,7 @@ func Test_codespace_displayName(t *testing.T) { DisplayName: "scuba steve", }, }, - args: args{ - includeName: false, - includeGitStatus: false, - }, - want: "cli/cli: trunk", + want: "cli/cli (trunk): scuba steve", }, { name: "No included name - included gitstatus - no unsaved changes", @@ -52,11 +44,7 @@ func Test_codespace_displayName(t *testing.T) { DisplayName: "scuba steve", }, }, - args: args{ - includeName: false, - includeGitStatus: true, - }, - want: "cli/cli: trunk", + want: "cli/cli (trunk): scuba steve", }, { name: "No included name - included gitstatus - unsaved changes", @@ -72,11 +60,7 @@ func Test_codespace_displayName(t *testing.T) { DisplayName: "scuba steve", }, }, - args: args{ - includeName: false, - includeGitStatus: true, - }, - want: "cli/cli: trunk*", + want: "cli/cli (trunk*): scuba steve", }, { name: "Included name - included gitstatus - unsaved changes", @@ -92,11 +76,7 @@ func Test_codespace_displayName(t *testing.T) { DisplayName: "scuba steve", }, }, - args: args{ - includeName: true, - includeGitStatus: true, - }, - want: "cli/cli: scuba steve (trunk*)", + want: "cli/cli (trunk*): scuba steve", }, { name: "Included name - included gitstatus - no unsaved changes", @@ -112,11 +92,7 @@ func Test_codespace_displayName(t *testing.T) { DisplayName: "scuba steve", }, }, - args: args{ - includeName: true, - includeGitStatus: true, - }, - want: "cli/cli: scuba steve (trunk)", + want: "cli/cli (trunk): scuba steve", }, } for _, tt := range tests { @@ -124,9 +100,166 @@ func Test_codespace_displayName(t *testing.T) { c := codespace{ Codespace: tt.fields.Codespace, } - if got := c.displayName(tt.args.includeName, tt.args.includeGitStatus); got != tt.want { + if got := c.displayName(); got != tt.want { t.Errorf("codespace.displayName() = %v, want %v", got, tt.want) } }) } } + +func Test_formatCodespacesForSelect(t *testing.T) { + type args struct { + codespaces []*api.Codespace + } + tests := []struct { + name string + args args + wantCodespacesNames []string + }{ + { + name: "One codespace: Shows only repo and branch name", + args: args{ + codespaces: []*api.Codespace{ + { + GitStatus: api.CodespaceGitStatus{ + Ref: "trunk", + }, + Repository: api.Repository{ + FullName: "cli/cli", + }, + DisplayName: "scuba steve", + }, + }, + }, + wantCodespacesNames: []string{ + "cli/cli (trunk): scuba steve", + }, + }, + { + name: "Two codespaces on the same repo/branch: Adds the codespace's display name", + args: args{ + codespaces: []*api.Codespace{ + { + GitStatus: api.CodespaceGitStatus{ + Ref: "trunk", + }, + Repository: api.Repository{ + FullName: "cli/cli", + }, + DisplayName: "scuba steve", + }, + { + GitStatus: api.CodespaceGitStatus{ + Ref: "trunk", + }, + Repository: api.Repository{ + FullName: "cli/cli", + }, + DisplayName: "flappy bird", + }, + }, + }, + wantCodespacesNames: []string{ + "cli/cli (trunk): scuba steve", + "cli/cli (trunk): flappy bird", + }, + }, + { + name: "Two codespaces on the different branches: Shows only repo and branch name", + args: args{ + codespaces: []*api.Codespace{ + { + GitStatus: api.CodespaceGitStatus{ + Ref: "trunk", + }, + Repository: api.Repository{ + FullName: "cli/cli", + }, + DisplayName: "scuba steve", + }, + { + GitStatus: api.CodespaceGitStatus{ + Ref: "feature", + }, + Repository: api.Repository{ + FullName: "cli/cli", + }, + DisplayName: "flappy bird", + }, + }, + }, + wantCodespacesNames: []string{ + "cli/cli (trunk): scuba steve", + "cli/cli (feature): flappy bird", + }, + }, + { + name: "Two codespaces on the different repos: Shows only repo and branch name", + args: args{ + codespaces: []*api.Codespace{ + { + GitStatus: api.CodespaceGitStatus{ + Ref: "trunk", + }, + Repository: api.Repository{ + FullName: "github/cli", + }, + DisplayName: "scuba steve", + }, + { + GitStatus: api.CodespaceGitStatus{ + Ref: "trunk", + }, + Repository: api.Repository{ + FullName: "cli/cli", + }, + DisplayName: "flappy bird", + }, + }, + }, + wantCodespacesNames: []string{ + "github/cli (trunk): scuba steve", + "cli/cli (trunk): flappy bird", + }, + }, + { + name: "Two codespaces on the same repo/branch, one dirty: Adds the codespace's display name and *", + args: args{ + codespaces: []*api.Codespace{ + { + GitStatus: api.CodespaceGitStatus{ + Ref: "trunk", + }, + Repository: api.Repository{ + FullName: "cli/cli", + }, + DisplayName: "scuba steve", + }, + { + GitStatus: api.CodespaceGitStatus{ + Ref: "trunk", + HasUncommitedChanges: true, + }, + Repository: api.Repository{ + FullName: "cli/cli", + }, + DisplayName: "flappy bird", + }, + }, + }, + wantCodespacesNames: []string{ + "cli/cli (trunk): scuba steve", + "cli/cli (trunk*): flappy bird", + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotCodespacesNames := formatCodespacesForSelect(tt.args.codespaces) + + if !reflect.DeepEqual(gotCodespacesNames, tt.wantCodespacesNames) { + t.Errorf("codespacesNames: got %v, want %v", gotCodespacesNames, tt.wantCodespacesNames) + } + }) + } +}