Repository: seachicken/gh-poi
Pull request #6 (2021-10-31 13:56:04)

Add error handling


Patch:
diff --git a/main.go b/main.go
index bfbdf0b..05f01cd 100644
--- a/main.go
+++ b/main.go
@@ -10,13 +10,19 @@ import (
 	"github.com/fatih/color"
 )
 
+var (
+	white     = color.New(color.FgWhite).SprintFunc()
+	whiteBold = color.New(color.FgWhite, color.Bold).SprintFunc()
+	hiBlack   = color.New(color.FgHiBlack).SprintFunc()
+	green     = color.New(color.FgGreen).SprintFunc()
+	red       = color.New(color.FgRed).SprintFunc()
+)
+
 func main() {
 	var check bool
 	flag.BoolVar(&check, "check", false, "Show branches to delete")
 	flag.Parse()
 
-	whiteBold := color.New(color.FgWhite, color.Bold).SprintFunc()
-
 	if check {
 		fmt.Fprintf(color.Output, "%s\n", whiteBold("== DRY RUN =="))
 	}
@@ -26,19 +32,42 @@ func main() {
 
 	conn := &ConnectionImpl{}
 
-	sp.Suffix = " Fetching pull requests..."
-	branches, err := GetBranches(conn)
-	if err != nil {
-		fmt.Fprintln(os.Stderr, err)
-	}
+	fetchingMsg := " Fetching pull requests..."
+	var fetchingErr error
+	sp.Suffix = fetchingMsg
+	branches, fetchingErr := GetBranches(conn)
 
-	if !check {
-		sp.Suffix = " Deleting branches..."
-		branches = DeleteBranches(branches, conn)
+	deletingMsg := " Deleting branches..."
+	var deletingErr error
+	if !check && fetchingErr == nil {
+		sp.Suffix = deletingMsg
+		branches, deletingErr = DeleteBranches(branches, conn)
 	}
 
 	sp.Stop()
 
+	if fetchingErr == nil {
+		fmt.Fprintf(color.Output, "%s%s\n", green("✔"), fetchingMsg)
+	} else {
+		fmt.Fprintf(color.Output, "%s%s\n", red("✕"), fetchingMsg)
+		fmt.Fprintln(os.Stderr, fetchingErr)
+		return
+	}
+
+	if check {
+		fmt.Fprintf(color.Output, "%s%s\n", hiBlack("-"), deletingMsg)
+	} else {
+		if deletingErr == nil {
+			fmt.Fprintf(color.Output, "%s%s\n", green("✔"), deletingMsg)
+		} else {
+			fmt.Fprintf(color.Output, "%s%s\n", red("✕"), deletingMsg)
+			fmt.Fprintln(os.Stderr, deletingErr)
+			return
+		}
+	}
+
+	fmt.Println()
+
 	var deletedStates []BranchState
 	var notDeletedStates []BranchState
 	if check {
@@ -59,15 +88,11 @@ func main() {
 }
 
 func printBranches(branches []Branch) {
-	hiBlack := color.New(color.FgHiBlack).SprintFunc()
-
 	if len(branches) == 0 {
-		fmt.Fprintf(color.Output, "%s\n", hiBlack("  There are no branches in the current directory"))
+		fmt.Fprintf(color.Output, "%s\n",
+			hiBlack("  There are no branches in the current directory"))
 	}
 
-	white := color.New(color.FgWhite).SprintFunc()
-	green := color.New(color.FgGreen).SprintFunc()
-
 	for _, branch := range branches {
 		branchName := fmt.Sprintf("%s", branch.Name)
 		if branch.Head {
diff --git a/mocks/poi_mock.go b/mocks/poi_mock.go
index f83fdf3..4ecaf29 100644
--- a/mocks/poi_mock.go
+++ b/mocks/poi_mock.go
@@ -34,11 +34,12 @@ func (m *MockConnection) EXPECT() *MockConnectionMockRecorder {
 }
 
 // DeleteBranches mocks base method.
-func (m *MockConnection) DeleteBranches(branchNames []string) string {
+func (m *MockConnection) DeleteBranches(branchNames []string) (string, error) {
 	m.ctrl.T.Helper()
 	ret := m.ctrl.Call(m, "DeleteBranches", branchNames)
 	ret0, _ := ret[0].(string)
-	return ret0
+	ret1, _ := ret[1].(error)
+	return ret0, ret1
 }
 
 // DeleteBranches indicates an expected call of DeleteBranches.
@@ -48,11 +49,12 @@ func (mr *MockConnectionMockRecorder) DeleteBranches(branchNames interface{}) *g
 }
 
 // FetchPrStates mocks base method.
-func (m *MockConnection) FetchPrStates(hostname string, repoNames []string, queryHashes string) string {
+func (m *MockConnection) FetchPrStates(hostname string, repoNames []string, queryHashes string) (string, error) {
 	m.ctrl.T.Helper()
 	ret := m.ctrl.Call(m, "FetchPrStates", hostname, repoNames, queryHashes)
 	ret0, _ := ret[0].(string)
-	return ret0
+	ret1, _ := ret[1].(error)
+	return ret0, ret1
 }
 
 // FetchPrStates indicates an expected call of FetchPrStates.
@@ -62,11 +64,12 @@ func (mr *MockConnectionMockRecorder) FetchPrStates(hostname, repoNames, queryHa
 }
 
 // FetchRepoNames mocks base method.
-func (m *MockConnection) FetchRepoNames() string {
+func (m *MockConnection) FetchRepoNames() (string, error) {
 	m.ctrl.T.Helper()
 	ret := m.ctrl.Call(m, "FetchRepoNames")
 	ret0, _ := ret[0].(string)
-	return ret0
+	ret1, _ := ret[1].(error)
+	return ret0, ret1
 }
 
 // FetchRepoNames indicates an expected call of FetchRepoNames.
@@ -76,11 +79,12 @@ func (mr *MockConnectionMockRecorder) FetchRepoNames() *gomock.Call {
 }
 
 // GetBrancheNames mocks base method.
-func (m *MockConnection) GetBrancheNames() string {
+func (m *MockConnection) GetBrancheNames() (string, error) {
 	m.ctrl.T.Helper()
 	ret := m.ctrl.Call(m, "GetBrancheNames")
 	ret0, _ := ret[0].(string)
-	return ret0
+	ret1, _ := ret[1].(error)
+	return ret0, ret1
 }
 
 // GetBrancheNames indicates an expected call of GetBrancheNames.
@@ -90,11 +94,12 @@ func (mr *MockConnectionMockRecorder) GetBrancheNames() *gomock.Call {
 }
 
 // GetRemoteName mocks base method.
-func (m *MockConnection) GetRemoteName() string {
+func (m *MockConnection) GetRemoteName() (string, error) {
 	m.ctrl.T.Helper()
 	ret := m.ctrl.Call(m, "GetRemoteName")
 	ret0, _ := ret[0].(string)
-	return ret0
+	ret1, _ := ret[1].(error)
+	return ret0, ret1
 }
 
 // GetRemoteName indicates an expected call of GetRemoteName.
diff --git a/poi.go b/poi.go
index 9120008..cbadc59 100644
--- a/poi.go
+++ b/poi.go
@@ -15,11 +15,11 @@ import (
 
 type (
 	Connection interface {
-		GetRemoteName() string
-		GetBrancheNames() string
-		DeleteBranches(branchNames []string) string
-		FetchRepoNames() string
-		FetchPrStates(hostname string, repoNames []string, queryHashes string) string
+		GetRemoteName() (string, error)
+		GetBrancheNames() (string, error)
+		DeleteBranches(branchNames []string) (string, error)
+		FetchRepoNames() (string, error)
+		FetchPrStates(hostname string, repoNames []string, queryHashes string) (string, error)
 	}
 
 	ConnectionImpl struct {
@@ -62,17 +62,37 @@ const (
 var ErrNotFound = errors.New("not found")
 
 func GetBranches(conn Connection) ([]Branch, error) {
-	hostname := getHostname(conn.GetRemoteName())
-	repoNames := strings.Split(conn.FetchRepoNames(), ",")
-	branches := toBranch(strings.Split(conn.GetBrancheNames(), "\n"))
+	var hostname string
+	if name, err := conn.GetRemoteName(); err == nil {
+		hostname = getHostname(name)
+	} else {
+		return nil, err
+	}
+
+	var repoNames []string
+	if names, err := conn.FetchRepoNames(); err == nil {
+		repoNames = strings.Split(names, ",")
+	} else {
+		return nil, err
+	}
+
+	var branches []Branch
+	if names, err := conn.GetBrancheNames(); err == nil {
+		branches = toBranch(strings.Split(names, "\n"))
+	} else {
+		return nil, err
+	}
 
 	prs := []PullRequest{}
 	for _, queryHashes := range GetQueryHashes(branches) {
-		pr, err := fromJson(conn.FetchPrStates(hostname, repoNames, queryHashes))
+		states, err := conn.FetchPrStates(hostname, repoNames, queryHashes)
 		if err != nil {
-			return []Branch{}, err
+			return nil, err
+		}
+
+		if pr, err := fromJson(states); err == nil {
+			prs = append(prs, pr...)
 		}
-		prs = append(prs, pr...)
 	}
 
 	branches = applyPullRequest(branches, prs)
@@ -183,22 +203,21 @@ func fromJson(jsonResp string) ([]PullRequest, error) {
 	}
 
 	var resp response
-	err := json.Unmarshal([]byte(jsonResp), &resp)
-	if err != nil {
-		return []PullRequest{}, err
+	if err := json.Unmarshal([]byte(jsonResp), &resp); err != nil {
+		return nil, fmt.Errorf("error unmarshaling response: %w", err)
 	}
 
 	results := []PullRequest{}
 	for _, edge := range resp.Data.Search.Edges {
 		state, err := toPullRequestState(edge.Node.State)
-		if err != nil {
-			return []PullRequest{}, err
+		if err == ErrNotFound {
+			return nil, fmt.Errorf("unexpected pull request state: %s", edge.Node.State)
 		}
 
 		results = append(results, PullRequest{
 			edge.Node.HeadRefName,
-			state, edge.Node.Number,
-			edge.Node.Url, edge.Node.Author.Login,
+			state,
+			edge.Node.Number, edge.Node.Url, edge.Node.Author.Login,
 		})
 	}
 
@@ -218,17 +237,21 @@ func toPullRequestState(state string) (PullRequestState, error) {
 	}
 }
 
-func DeleteBranches(branches []Branch, conn Connection) []Branch {
+func DeleteBranches(branches []Branch, conn Connection) ([]Branch, error) {
 	branchNames := getBranchNames(branches, Deletable)
 	if len(branchNames) == 0 {
-		return branches
+		return branches, nil
 	}
 
 	conn.DeleteBranches(branchNames)
 
-	branchesAfter := toBranch(strings.Split(conn.GetBrancheNames(), "\n"))
+	branchNamesAfter, err := conn.GetBrancheNames()
+	if err != nil {
+		return nil, err
+	}
+	branchesAfter := toBranch(strings.Split(branchNamesAfter, "\n"))
 
-	return checkDeleted(branches, branchesAfter)
+	return checkDeleted(branches, branchesAfter), nil
 }
 
 func getBranchNames(branches []Branch, state BranchState) []string {
@@ -241,12 +264,11 @@ func getBranchNames(branches []Branch, state BranchState) []string {
 	return results
 }
 
-func checkDeleted(branches []Branch, branchesAfter []Branch) []Branch {
+func checkDeleted(branchesBefore []Branch, branchesAfter []Branch) []Branch {
 	results := []Branch{}
-	for _, branch := range branches {
+	for _, branch := range branchesBefore {
 		if branch.State == Deletable {
-			_, err := findMatchedBranch(branch.Name, branchesAfter)
-			if err == ErrNotFound {
+			if !branchNameExists(branch.Name, branchesAfter) {
 				branch.State = Deleted
 			}
 		}
@@ -255,44 +277,38 @@ func checkDeleted(branches []Branch, branchesAfter []Branch) []Branch {
 	return results
 }
 
-func findMatchedBranch(branchName string, branches []Branch) (Branch, error) {
+func branchNameExists(branchName string, branches []Branch) bool {
 	for _, branch := range branches {
 		if branch.Name == branchName {
-			return branch, nil
+			return true
 		}
 	}
-	return Branch{}, ErrNotFound
+	return false
 }
 
-func (conn *ConnectionImpl) GetRemoteName() string {
+func (conn *ConnectionImpl) GetRemoteName() (string, error) {
 	args := []string{
 		"remote", "-v",
 	}
-	stdout, _, _ := run("git", args)
-
-	return stdout.String()
+	return run("git", args)
 }
 
-func (conn *ConnectionImpl) GetBrancheNames() string {
+func (conn *ConnectionImpl) GetBrancheNames() (string, error) {
 	args := []string{
 		"branch", "-v", "--no-abbrev",
 		"--format=%(HEAD),%(refname:lstrip=2),%(objectname)",
 	}
-	stdout, _, _ := run("git", args)
-
-	return stdout.String()
+	return run("git", args)
 }
 
-func (conn *ConnectionImpl) DeleteBranches(branchNames []string) string {
+func (conn *ConnectionImpl) DeleteBranches(branchNames []string) (string, error) {
 	args := append([]string{
 		"branch", "-D"},
 		branchNames...)
-	stdout, _, _ := run("git", args)
-
-	return stdout.String()
+	return run("git", args)
 }
 
-func (conn *ConnectionImpl) FetchRepoNames() string {
+func (conn *ConnectionImpl) FetchRepoNames() (string, error) {
 	args := []string{
 		"repo", "view",
 		"--json", "owner",
@@ -300,13 +316,11 @@ func (conn *ConnectionImpl) FetchRepoNames() string {
 		"--json", "parent",
 		"--template", "{{ .owner.login }}/{{ .name }}{{ if.parent }},{{ .parent.owner.login }}/{{ .parent.name }}{{ end }}",
 	}
-	stdout, _, _ := run("gh", args)
-
-	return stdout.String()
+	return run("gh", args)
 }
 
 func (conn *ConnectionImpl) FetchPrStates(
-	hostname string, repoNames []string, queryHashes string) string {
+	hostname string, repoNames []string, queryHashes string) (string, error) {
 	args := []string{
 		"api", "graphql",
 		"--hostname", hostname,
@@ -330,9 +344,7 @@ func (conn *ConnectionImpl) FetchPrStates(
 			queryHashes,
 		),
 	}
-	stdout, _, _ := run("gh", args)
-
-	return stdout.String()
+	return run("gh", args)
 }
 
 func getQueryRepos(repoNames []string) string {
@@ -343,20 +355,21 @@ func getQueryRepos(repoNames []string) string {
 	return repos.String()
 }
 
-func run(file string, args []string) (stdout, stderr bytes.Buffer, err error) {
+func run(file string, args []string) (string, error) {
 	bin, err := safeexec.LookPath(file)
 	if err != nil {
-		return
+		return "", fmt.Errorf("failed to run external command: %s", file)
 	}
 
+	var stdout, stderr bytes.Buffer
 	cmd := exec.Command(bin, args...)
 	cmd.Stdout = &stdout
 	cmd.Stderr = &stderr
 
 	err = cmd.Run()
 	if err != nil {
-		return
+		return "", fmt.Errorf("failed to run external command: %s", file)
 	}
 
-	return
+	return stdout.String(), nil
 }


Test patch:
diff --git a/poi_test.go b/poi_test.go
index 759913c..ae00b3e 100644
--- a/poi_test.go
+++ b/poi_test.go
@@ -18,19 +18,19 @@ func TestGetBranches(t *testing.T) {
 		GetRemoteName().
 		Return(`origin  git@github.com:owner/name.git (fetch)
 origin  git@github.com:owner/name.git (push)
-`).
+`, nil).
 		AnyTimes()
 	conn.
 		EXPECT().
 		GetBrancheNames().
 		Return(` ,issue1,356a192b7913b04c54574d18c28d46e6395428ab
 *,main,b6589fc6ab0dc82cf12099d1c2d40ab994e8410c
-`).
+`, nil).
 		AnyTimes()
 	conn.
 		EXPECT().
 		FetchRepoNames().
-		Return("owner/name").
+		Return("owner/name", nil).
 		AnyTimes()
 	conn.
 		EXPECT().
@@ -54,7 +54,7 @@ origin  git@github.com:owner/name.git (push)
       ]
     }
   }
-}`).
+}`, nil).
 		AnyTimes()
 
 	actual, _ := GetBranches(conn)
@@ -83,19 +83,19 @@ func TestGetBranches_pullRequestsAreNotMerged(t *testing.T) {
 		GetRemoteName().
 		Return(`origin  git@github.com:owner/name.git (fetch)
 origin  git@github.com:owner/name.git (push)
-`).
+`, nil).
 		AnyTimes()
 	conn.
 		EXPECT().
 		GetBrancheNames().
 		Return(` ,issue1,356a192b7913b04c54574d18c28d46e6395428ab
 *,main,b6589fc6ab0dc82cf12099d1c2d40ab994e8410c
-`).
+`, nil).
 		AnyTimes()
 	conn.
 		EXPECT().
 		FetchRepoNames().
-		Return("owner/name").
+		Return("owner/name", nil).
 		AnyTimes()
 	conn.
 		EXPECT().
@@ -130,7 +130,7 @@ origin  git@github.com:owner/name.git (push)
       ]
     }
   }
-}`).
+}`, nil).
 		AnyTimes()
 
 	actual, _ := GetBranches(conn)
@@ -160,12 +160,12 @@ func TestDeleteBranches(t *testing.T) {
 		GetBrancheNames().
 		Return(`*,issue1,356a192b7913b04c54574d18c28d46e6395428ab
  ,main,b6589fc6ab0dc82cf12099d1c2d40ab994e8410c
-`).
+`, nil).
 		AnyTimes()
 	conn.
 		EXPECT().
 		DeleteBranches(gomock.Any()).
-		Return("").
+		Return("", nil).
 		Times(1)
 
 	branches := []Branch{
@@ -174,12 +174,14 @@ func TestDeleteBranches(t *testing.T) {
 		{true, "main", "", []PullRequest{}, NotDeletable},
 	}
 
+	actual, _ := DeleteBranches(branches, conn)
+
 	expected := []Branch{
 		{false, "issue1", "", []PullRequest{}, Deletable},
 		{false, "issue2", "", []PullRequest{}, Deleted},
 		{true, "main", "", []PullRequest{}, NotDeletable},
 	}
-	assert.Equal(t, expected, DeleteBranches(branches, conn))
+	assert.Equal(t, expected, actual)
 }
 
 func TestDeleteBranches_doesNotExistsDeletableBranches(t *testing.T) {
@@ -192,12 +194,12 @@ func TestDeleteBranches_doesNotExistsDeletableBranches(t *testing.T) {
 		GetBrancheNames().
 		Return(` ,issue1,356a192b7913b04c54574d18c28d46e6395428ab
 *,main,b6589fc6ab0dc82cf12099d1c2d40ab994e8410c
-`).
+`, nil).
 		AnyTimes()
 	conn.
 		EXPECT().
 		DeleteBranches(gomock.Any()).
-		Return("").
+		Return("", nil).
 		Times(0)
 
 	branches := []Branch{