Changed parsefileArg to return a string, reformatted testing, polished up browse.go

jlsestak · jlsestak · commit b64488fe5cd0 · 2021-06-15T20:10:48.000-07:00
diff --git a/pkg/cmd/browse/browse.go b/pkg/cmd/browse/browse.go
@@ -107,12 +107,12 @@ func NewCmdBrowse(f *cmdutil.Factory, runF func(*BrowseOptions) error) *cobra.Co
 func runBrowse(opts *BrowseOptions) error {
 	baseRepo, err := opts.BaseRepo()
 	if err != nil {
-		return fmt.Errorf("unable to determine base repository: %w\nUse 'gh browse --help' for more information about browse\n", err)
+		return fmt.Errorf("unable to determine base repository: %w\n", err)
 	}
 
 	httpClient, err := opts.HttpClient()
 	if err != nil {
-		return fmt.Errorf("unable to create an http client: %w\nUse 'gh browse --help' for more information about browse\n", err)
+		return fmt.Errorf("unable to create an http client: %w\n", err)
 	}
 	url := ghrepo.GenerateRepoURL(baseRepo, "")
 
@@ -133,7 +133,7 @@ func runBrowse(opts *BrowseOptions) error {
 		if isNumber(opts.SelectorArg) {
 			url += "/issues/" + opts.SelectorArg
 		} else {
-			arr, err := parseFileArg(opts.SelectorArg)
+			fileArg, err := parseFileArg(opts.SelectorArg)
 			if err != nil {
 				return err
 			}
@@ -147,33 +147,28 @@ func runBrowse(opts *BrowseOptions) error {
 				}
 				url += "/tree/" + branchName + "/"
 			}
-			if opts.SelectorArg != "" {
-				if len(arr) > 1 {
-					url += arr[0] + "#L" + arr[1]
-				} else {
-					url += arr[0]
-				}
-			}
+			url += fileArg
 		}
 	}
 
-	err = opts.Browser.Browse(url)
-	if opts.IO.IsStdoutTTY() && err == nil {
+	if opts.IO.IsStdoutTTY() {
 		fmt.Fprintf(opts.IO.Out, "now opening %s in browser\n", url)
 	}
-
-	return err
+	return opts.Browser.Browse(url)
 }
 
-func parseFileArg(fileArg string) ([]string, error) {
+func parseFileArg(fileArg string) (string, error) {
 	arr := strings.Split(fileArg, ":")
 	if len(arr) > 2 {
-		return arr, fmt.Errorf("invalid use of colon\nUse 'gh browse --help' for more information about browse\n")
+		return "", fmt.Errorf("invalid use of colon\nUse 'gh browse --help' for more information about browse\n")
 	}
-	if len(arr) > 1 && !isNumber(arr[1]) {
-		return arr, fmt.Errorf("invalid line number after colon\nUse 'gh browse --help' for more information about browse\n")
+	if len(arr) > 1 {
+		if !isNumber(arr[1]) {
+			return "", fmt.Errorf("invalid line number after colon\nUse 'gh browse --help' for more information about browse\n")
+		}
+		return arr[0] + "#L" + arr[1], nil
 	}
-	return arr, nil
+	return arr[0], nil
 }
 
 func isNumber(arg string) bool {
diff --git a/pkg/cmd/browse/browse_test.go b/pkg/cmd/browse/browse_test.go
@@ -271,27 +271,25 @@ func Test_runBrowse(t *testing.T) {
 	}
 }
 
-func TestFileArgParsing(t *testing.T) {
+func Test_parseFileArg(t *testing.T) {
 	tests := []struct {
-		name           string
-		arg            string
-		errorExpected  bool
-		fileArg        string
-		lineArg        string
-		stderrExpected string
+		name            string
+		arg             string
+		errorExpected   bool
+		expectedFileArg string
+		stderrExpected  string
 	}{
 		{
-			name:          "non line number",
-			arg:           "main.go",
-			errorExpected: false,
-			fileArg:       "main.go",
+			name:            "non line number",
+			arg:             "main.go",
+			errorExpected:   false,
+			expectedFileArg: "main.go",
 		},
 		{
-			name:          "line number",
-			arg:           "main.go:32",
-			errorExpected: false,
-			fileArg:       "main.go",
-			lineArg:       "32",
+			name:            "line number",
+			arg:             "main.go:32",
+			errorExpected:   false,
+			expectedFileArg: "main.go#L32",
 		},
 		{
 			name:           "non line number error",
@@ -301,15 +299,12 @@ func TestFileArgParsing(t *testing.T) {
 		},
 	}
 	for _, tt := range tests {
-		arr, err := parseFileArg(tt.arg)
+		fileArg, err := parseFileArg(tt.arg)
 		if tt.errorExpected {
 			assert.Equal(t, err.Error(), tt.stderrExpected)
 		} else {
 			assert.Equal(t, err, nil)
-			if len(arr) > 1 {
-				assert.Equal(t, tt.lineArg, arr[1])
-			}
-			assert.Equal(t, tt.fileArg, arr[0])
+			assert.Equal(t, tt.expectedFileArg, fileArg)
 		}
 	}
 }

Original file line number	Diff line number	Diff line change
`@@ -107,12 +107,12 @@ func NewCmdBrowse(f cmdutil.Factory, runF func(BrowseOptions) error) *cobra.Co`
`107`	`107`	`func runBrowse(opts *BrowseOptions) error {`
`108`	`108`	`baseRepo, err := opts.BaseRepo()`
`109`	`109`	`if err != nil {`
`110`		`- return fmt.Errorf("unable to determine base repository: %w\nUse 'gh browse --help' for more information about browse\n", err)`
	`110`	`+ return fmt.Errorf("unable to determine base repository: %w\n", err)`
`111`	`111`	`}`
`112`	`112`
`113`	`113`	`httpClient, err := opts.HttpClient()`
`114`	`114`	`if err != nil {`
`115`		`- return fmt.Errorf("unable to create an http client: %w\nUse 'gh browse --help' for more information about browse\n", err)`
	`115`	`+ return fmt.Errorf("unable to create an http client: %w\n", err)`
`116`	`116`	`}`
`117`	`117`	`url := ghrepo.GenerateRepoURL(baseRepo, "")`
`118`	`118`
`@@ -133,7 +133,7 @@ func runBrowse(opts *BrowseOptions) error {`
`133`	`133`	`if isNumber(opts.SelectorArg) {`
`134`	`134`	`url += "/issues/" + opts.SelectorArg`
`135`	`135`	`} else {`
`136`		`- arr, err := parseFileArg(opts.SelectorArg)`
	`136`	`+ fileArg, err := parseFileArg(opts.SelectorArg)`
`137`	`137`	`if err != nil {`
`138`	`138`	`return err`
`139`	`139`	`}`
`@@ -147,33 +147,28 @@ func runBrowse(opts *BrowseOptions) error {`
`147`	`147`	`}`
`148`	`148`	`url += "/tree/" + branchName + "/"`
`149`	`149`	`}`
`150`		`- if opts.SelectorArg != "" {`
`151`		`- if len(arr) > 1 {`
`152`		`- url += arr[0] + "#L" + arr[1]`
`153`		`- } else {`
`154`		`- url += arr[0]`
`155`		`- }`
`156`		`- }`
	`150`	`+ url += fileArg`
`157`	`151`	`}`
`158`	`152`	`}`
`159`	`153`
`160`		`- err = opts.Browser.Browse(url)`
`161`		`- if opts.IO.IsStdoutTTY() && err == nil {`
	`154`	`+ if opts.IO.IsStdoutTTY() {`
`162`	`155`	`fmt.Fprintf(opts.IO.Out, "now opening %s in browser\n", url)`
`163`	`156`	`}`
`164`		`-`
`165`		`- return err`
	`157`	`+ return opts.Browser.Browse(url)`
`166`	`158`	`}`
`167`	`159`
`168`		`-func parseFileArg(fileArg string) ([]string, error) {`
	`160`	`+func parseFileArg(fileArg string) (string, error) {`
`169`	`161`	`arr := strings.Split(fileArg, ":")`
`170`	`162`	`if len(arr) > 2 {`
`171`		`- return arr, fmt.Errorf("invalid use of colon\nUse 'gh browse --help' for more information about browse\n")`
	`163`	`+ return "", fmt.Errorf("invalid use of colon\nUse 'gh browse --help' for more information about browse\n")`
`172`	`164`	`}`
`173`		`- if len(arr) > 1 && !isNumber(arr[1]) {`
`174`		`- return arr, fmt.Errorf("invalid line number after colon\nUse 'gh browse --help' for more information about browse\n")`
	`165`	`+ if len(arr) > 1 {`
	`166`	`+ if !isNumber(arr[1]) {`
	`167`	`+ return "", fmt.Errorf("invalid line number after colon\nUse 'gh browse --help' for more information about browse\n")`
	`168`	`+ }`
	`169`	`+ return arr[0] + "#L" + arr[1], nil`
`175`	`170`	`}`
`176`		`- return arr, nil`
	`171`	`+ return arr[0], nil`
`177`	`172`	`}`
`178`	`173`
`179`	`174`	`func isNumber(arg string) bool {`