(refactor) consolidate argument parsing logic

zaquestion · Dec 23, 2017 · 2531851 · 2531851
1 parent 51e926b
commit 2531851
Show file tree

Hide file tree

Showing 12 changed files with 108 additions and 91 deletions.
diff --git a/cmd/issueBrowse.go b/cmd/issueBrowse.go
@@ -9,7 +9,6 @@ import (
 	"github.com/spf13/cobra"
 	"github.com/spf13/viper"
 	"github.com/zaquestion/lab/internal/browser"
-	"github.com/zaquestion/lab/internal/git"
 )
 
 var browse = browser.Open
@@ -20,14 +19,7 @@ var issueBrowseCmd = &cobra.Command{
 	Short:   "View issue in a browser",
 	Long:    ``,
 	Run: func(cmd *cobra.Command, args []string) {
-		remote, num, err := parseArgsRemote(args)
-		if err != nil {
-			log.Fatal(err)
-		}
-		if remote == "" {
-			remote = forkedFromRemote
-		}
-		rn, err := git.PathWithNameSpace(remote)
+		rn, num, err := parseArgs(args)
 		if err != nil {
 			log.Fatal(err)
 		}

diff --git a/cmd/issueClose.go b/cmd/issueClose.go
@@ -5,7 +5,6 @@ import (
 	"log"
 
 	"github.com/spf13/cobra"
-	"github.com/zaquestion/lab/internal/git"
 	lab "github.com/zaquestion/lab/internal/gitlab"
 )
 
@@ -16,14 +15,7 @@ var issueCloseCmd = &cobra.Command{
 	Long:    ``,
 	Args:    cobra.MinimumNArgs(1),
 	Run: func(cmd *cobra.Command, args []string) {
-		remote, id, err := parseArgsRemote(args)
-		if err != nil {
-			log.Fatal(err)
-		}
-		if remote == "" {
-			remote = forkedFromRemote
-		}
-		rn, err := git.PathWithNameSpace(remote)
+		rn, id, err := parseArgs(args)
 		if err != nil {
 			log.Fatal(err)
 		}

diff --git a/cmd/issueList.go b/cmd/issueList.go
@@ -6,7 +6,6 @@ import (
 
 	"github.com/spf13/cobra"
 	"github.com/xanzy/go-gitlab"
-	"github.com/zaquestion/lab/internal/git"
 	lab "github.com/zaquestion/lab/internal/gitlab"
 )
 
@@ -21,14 +20,7 @@ var issueListCmd = &cobra.Command{
 	Short:   "List issues",
 	Long:    ``,
 	Run: func(cmd *cobra.Command, args []string) {
-		remote, page, err := parseArgsRemote(args)
-		if err != nil {
-			log.Fatal(err)
-		}
-		if remote == "" {
-			remote = forkedFromRemote
-		}
-		rn, err := git.PathWithNameSpace(remote)
+		rn, page, err := parseArgs(args)
 		if err != nil {
 			log.Fatal(err)
 		}

diff --git a/cmd/issueShow.go b/cmd/issueShow.go
@@ -8,7 +8,6 @@ import (
 
 	"github.com/spf13/cobra"
 	"github.com/xanzy/go-gitlab"
-	"github.com/zaquestion/lab/internal/git"
 	lab "github.com/zaquestion/lab/internal/gitlab"
 )
 
@@ -19,14 +18,7 @@ var issueShowCmd = &cobra.Command{
 	Short:      "Describe an issue",
 	Long:       ``,
 	Run: func(cmd *cobra.Command, args []string) {
-		remote, issueNum, err := parseArgsRemote(args)
-		if err != nil {
-			log.Fatal(err)
-		}
-		if remote == "" {
-			remote = forkedFromRemote
-		}
-		rn, err := git.PathWithNameSpace(remote)
+		rn, issueNum, err := parseArgs(args)
 		if err != nil {
 			log.Fatal(err)
 		}

diff --git a/cmd/mrBrowse.go b/cmd/mrBrowse.go
@@ -8,7 +8,6 @@ import (
 
 	"github.com/spf13/cobra"
 	"github.com/spf13/viper"
-	"github.com/zaquestion/lab/internal/git"
 )
 
 var mrBrowseCmd = &cobra.Command{
@@ -17,14 +16,7 @@ var mrBrowseCmd = &cobra.Command{
 	Short:   "View merge request in a browser",
 	Long:    ``,
 	Run: func(cmd *cobra.Command, args []string) {
-		remote, num, err := parseArgsRemote(args)
-		if err != nil {
-			log.Fatal(err)
-		}
-		if remote == "" {
-			remote = forkedFromRemote
-		}
-		rn, err := git.PathWithNameSpace(remote)
+		rn, num, err := parseArgs(args)
 		if err != nil {
 			log.Fatal(err)
 		}

diff --git a/cmd/mrCheckout.go b/cmd/mrCheckout.go
@@ -3,7 +3,6 @@ package cmd
 import (
 	"fmt"
 	"log"
-	"strconv"
 
 	"github.com/spf13/cobra"
 	"github.com/xanzy/go-gitlab"
@@ -18,28 +17,24 @@ var checkoutCmd = &cobra.Command{
 	Long:  ``,
 	Args:  cobra.ExactArgs(1),
 	Run: func(cmd *cobra.Command, args []string) {
-		rn, err := git.PathWithNameSpace(forkedFromRemote)
-		if err != nil {
-			log.Fatal(err)
-		}
-		mrIDStr := args[0]
-		mrID, err := strconv.Atoi(mrIDStr)
+		rn, mrID, err := parseArgs(args)
 		if err != nil {
 			log.Fatal(err)
 		}
+
 		mrs, err := lab.MRList(rn, &gitlab.ListProjectMergeRequestsOptions{
-			IIDs: []int{mrID},
+			IIDs: []int{int(mrID)},
 		})
 		if err != nil {
 			log.Fatal(err)
 		}
 		if len(mrs) < 1 {
-			fmt.Printf("MR #%s not found\n", mrIDStr)
+			fmt.Printf("MR #%d not found\n", mrID)
 			return
 		}
 		// https://docs.gitlab.com/ee/user/project/merge_requests/#checkout-merge-requests-locally
 		branch := mrs[0].SourceBranch
-		mr := fmt.Sprintf("refs/merge-requests/%s/head", mrIDStr)
+		mr := fmt.Sprintf("refs/merge-requests/%d/head", mrID)
 		gitf := git.New("fetch", forkedFromRemote, fmt.Sprintf("%s:%s", mr, branch))
 		err = gitf.Run()
 		if err != nil {

diff --git a/cmd/mrClose.go b/cmd/mrClose.go
@@ -5,7 +5,6 @@ import (
 	"log"
 
 	"github.com/spf13/cobra"
-	"github.com/zaquestion/lab/internal/git"
 	lab "github.com/zaquestion/lab/internal/gitlab"
 )
 
@@ -16,14 +15,7 @@ var mrCloseCmd = &cobra.Command{
 	Long:    ``,
 	Args:    cobra.MinimumNArgs(1),
 	Run: func(cmd *cobra.Command, args []string) {
-		remote, id, err := parseArgsRemote(args)
-		if err != nil {
-			log.Fatal(err)
-		}
-		if remote == "" {
-			remote = forkedFromRemote
-		}
-		rn, err := git.PathWithNameSpace(remote)
+		rn, id, err := parseArgs(args)
 		if err != nil {
 			log.Fatal(err)
 		}

diff --git a/cmd/mrList.go b/cmd/mrList.go
@@ -6,7 +6,6 @@ import (
 
 	"github.com/spf13/cobra"
 	"github.com/xanzy/go-gitlab"
-	"github.com/zaquestion/lab/internal/git"
 	lab "github.com/zaquestion/lab/internal/gitlab"
 )
 
@@ -21,14 +20,7 @@ var listCmd = &cobra.Command{
 	Long:    ``,
 	Args:    cobra.MaximumNArgs(1),
 	Run: func(cmd *cobra.Command, args []string) {
-		remote, page, err := parseArgsRemote(args)
-		if err != nil {
-			log.Fatal(err)
-		}
-		if remote == "" {
-			remote = forkedFromRemote
-		}
-		rn, err := git.PathWithNameSpace(remote)
+		rn, page, err := parseArgs(args)
 		if err != nil {
 			log.Fatal(err)
 		}

diff --git a/cmd/mrMerge.go b/cmd/mrMerge.go
@@ -4,7 +4,6 @@ import (
 	"log"
 
 	"github.com/spf13/cobra"
-	"github.com/zaquestion/lab/internal/git"
 	lab "github.com/zaquestion/lab/internal/gitlab"
 )
 
@@ -15,14 +14,7 @@ var mrMergeCmd = &cobra.Command{
 	Long:    `If the pipeline for the mr is still running, lab sets merge on success`,
 	Args:    cobra.MinimumNArgs(1),
 	Run: func(cmd *cobra.Command, args []string) {
-		remote, id, err := parseArgsRemote(args)
-		if err != nil {
-			log.Fatal(err)
-		}
-		if remote == "" {
-			remote = forkedFromRemote
-		}
-		rn, err := git.PathWithNameSpace(remote)
+		rn, id, err := parseArgs(args)
 		if err != nil {
 			log.Fatal(err)
 		}

diff --git a/cmd/mrShow.go b/cmd/mrShow.go
@@ -7,7 +7,6 @@ import (
 
 	"github.com/spf13/cobra"
 	"github.com/xanzy/go-gitlab"
-	"github.com/zaquestion/lab/internal/git"
 	lab "github.com/zaquestion/lab/internal/gitlab"
 )
 
@@ -18,14 +17,7 @@ var mrShowCmd = &cobra.Command{
 	Short:      "Describe a merge request",
 	Long:       ``,
 	Run: func(cmd *cobra.Command, args []string) {
-		remote, mrNum, err := parseArgsRemote(args)
-		if err != nil {
-			log.Fatal(err)
-		}
-		if remote == "" {
-			remote = forkedFromRemote
-		}
-		rn, err := git.PathWithNameSpace(remote)
+		rn, mrNum, err := parseArgs(args)
 		if err != nil {
 			log.Fatal(err)
 		}

diff --git a/cmd/root.go b/cmd/root.go
@@ -101,6 +101,21 @@ func parseArgsRemote(args []string) (string, int64, error) {
 	return "", 0, nil
 }
 
+func parseArgs(args []string) (string, int64, error) {
+	remote, num, err := parseArgsRemote(args)
+	if err != nil {
+		return "", 0, err
+	}
+	if remote == "" {
+		remote = forkedFromRemote
+	}
+	rn, err := git.PathWithNameSpace(remote)
+	if err != nil {
+		return "", 0, err
+	}
+	return rn, num, nil
+}
+
 var (
 	// Will be updated to upstream in init() if "upstream" remote exists
 	forkedFromRemote = "origin"

diff --git a/cmd/root_test.go b/cmd/root_test.go
@@ -174,3 +174,82 @@ func Test_parseArgsRemote(t *testing.T) {
 		})
 	}
 }
+
+func Test_parseArgs(t *testing.T) {
+	tests := []struct {
+		Name           string
+		Args           []string
+		ExpectedString string
+		ExpectedInt    int64
+		ExpectedErr    string
+	}{
+		{
+			Name:           "No Args",
+			Args:           nil,
+			ExpectedString: "zaquestion/test",
+			ExpectedInt:    0,
+			ExpectedErr:    "",
+		},
+		{
+			Name:           "1 arg remote",
+			Args:           []string{"lab-testing"},
+			ExpectedString: "lab-testing/test",
+			ExpectedInt:    0,
+			ExpectedErr:    "",
+		},
+		{
+			Name:           "1 arg non remote",
+			Args:           []string{"foo"},
+			ExpectedString: "",
+			ExpectedInt:    0,
+			ExpectedErr:    "foo is not a valid remote or number",
+		},
+		{
+			Name:           "1 arg page",
+			Args:           []string{"100"},
+			ExpectedString: "zaquestion/test",
+			ExpectedInt:    100,
+			ExpectedErr:    "",
+		},
+		{
+			Name:           "1 arg invalid page",
+			Args:           []string{"asdf100"},
+			ExpectedString: "",
+			ExpectedInt:    0,
+			ExpectedErr:    "asdf100 is not a valid remote or number",
+		},
+		{
+			Name:           "2 arg remote page",
+			Args:           []string{"origin", "100"},
+			ExpectedString: "zaquestion/test",
+			ExpectedInt:    100,
+			ExpectedErr:    "",
+		},
+		{
+			Name:           "2 arg invalid remote valid page",
+			Args:           []string{"foo", "100"},
+			ExpectedString: "",
+			ExpectedInt:    0,
+			ExpectedErr:    "foo is not a valid remote",
+		},
+		{
+			Name:           "2 arg valid remote invalid page",
+			Args:           []string{"foo", "asdf100"},
+			ExpectedString: "",
+			ExpectedInt:    0,
+			ExpectedErr:    "strconv.ParseInt: parsing \"asdf100\": invalid syntax",
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.Name, func(t *testing.T) {
+			test := test
+			t.Parallel()
+			s, i, err := parseArgs(test.Args)
+			if err != nil {
+				assert.EqualError(t, err, test.ExpectedErr)
+			}
+			assert.Equal(t, test.ExpectedString, s)
+			assert.Equal(t, test.ExpectedInt, i)
+		})
+	}
+}