Improved error message when "owner/repo" format not provided (cli#919)

kevinbluer · web-flow · commit d440a95aed76 · 2020-05-18T11:13:48.000+02:00
Fixes cli#882
diff --git a/api/queries_issue_test.go b/api/queries_issue_test.go
@@ -39,7 +39,8 @@ func TestIssueList(t *testing.T) {
 	} } }
 	`))
 
-	_, err := IssueList(client, ghrepo.FromFullName("OWNER/REPO"), "open", []string{}, "", 251, "")
+	repo, _ := ghrepo.FromFullName("OWNER/REPO")
+	_, err := IssueList(client, repo, "open", []string{}, "", 251, "")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
diff --git a/api/queries_repo_test.go b/api/queries_repo_test.go
@@ -50,7 +50,7 @@ func Test_RepoMetadata(t *testing.T) {
 	http := &httpmock.Registry{}
 	client := NewClient(ReplaceTripper(http))
 
-	repo := ghrepo.FromFullName("OWNER/REPO")
+	repo, _ := ghrepo.FromFullName("OWNER/REPO")
 	input := RepoMetadataInput{
 		Assignees:  true,
 		Reviewers:  true,
@@ -181,7 +181,7 @@ func Test_RepoResolveMetadataIDs(t *testing.T) {
 	http := &httpmock.Registry{}
 	client := NewClient(ReplaceTripper(http))
 
-	repo := ghrepo.FromFullName("OWNER/REPO")
+	repo, _ := ghrepo.FromFullName("OWNER/REPO")
 	input := RepoResolveInput{
 		Assignees: []string{"monalisa", "hubot"},
 		Reviewers: []string{"monalisa", "octocat", "OWNER/core", "/robots"},
diff --git a/command/repo.go b/command/repo.go
@@ -189,7 +189,10 @@ func repoCreate(cmd *cobra.Command, args []string) error {
 	if len(args) > 0 {
 		name = args[0]
 		if strings.Contains(name, "/") {
-			newRepo := ghrepo.FromFullName(name)
+			newRepo, err := ghrepo.FromFullName(name)
+			if err != nil {
+				return fmt.Errorf("argument error: %w", err)
+			}
 			orgName = newRepo.RepoOwner()
 			name = newRepo.RepoName()
 		}
@@ -366,9 +369,9 @@ func repoFork(cmd *cobra.Command, args []string) error {
 				return fmt.Errorf("did not understand argument: %w", err)
 			}
 		} else {
-			repoToFork = ghrepo.FromFullName(repoArg)
-			if repoToFork.RepoName() == "" || repoToFork.RepoOwner() == "" {
-				return fmt.Errorf("could not parse owner or repo name from %s", repoArg)
+			repoToFork, err = ghrepo.FromFullName(repoArg)
+			if err != nil {
+				return fmt.Errorf("argument error: %w", err)
 			}
 		}
 	}
@@ -508,7 +511,11 @@ func repoView(cmd *cobra.Command, args []string) error {
 				return fmt.Errorf("did not understand argument: %w", err)
 			}
 		} else {
-			toView = ghrepo.FromFullName(repoArg)
+			var err error
+			toView, err = ghrepo.FromFullName(repoArg)
+			if err != nil {
+				return fmt.Errorf("argument error: %w", err)
+			}
 		}
 	}
 
diff --git a/command/root.go b/command/root.go
@@ -219,7 +219,11 @@ func changelogURL(version string) string {
 func determineBaseRepo(cmd *cobra.Command, ctx context.Context) (ghrepo.Interface, error) {
 	repo, err := cmd.Flags().GetString("repo")
 	if err == nil && repo != "" {
-		return ghrepo.FromFullName(repo), nil
+		baseRepo, err := ghrepo.FromFullName(repo)
+		if err != nil {
+			return nil, fmt.Errorf("argument error: %w", err)
+		}
+		return baseRepo, nil
 	}
 
 	apiClient, err := apiClientForContext(ctx)
diff --git a/context/blank_context.go b/context/blank_context.go
@@ -92,5 +92,6 @@ func (c *blankContext) BaseRepo() (ghrepo.Interface, error) {
 }
 
 func (c *blankContext) SetBaseRepo(nwo string) {
-	c.baseRepo = ghrepo.FromFullName(nwo)
+	repo, _ := ghrepo.FromFullName(nwo)
+	c.baseRepo = repo
 }
diff --git a/context/context.go b/context/context.go
@@ -39,7 +39,7 @@ func ResolveRemotesToRepos(remotes Remotes, client *api.Client, base string) (Re
 	}
 
 	hasBaseOverride := base != ""
-	baseOverride := ghrepo.FromFullName(base)
+	baseOverride, _ := ghrepo.FromFullName(base)
 	foundBaseOverride := false
 	repos := make([]ghrepo.Interface, 0, lenRemotesForLookup)
 	for _, r := range remotes[:lenRemotesForLookup] {
@@ -266,5 +266,5 @@ func (c *fsContext) BaseRepo() (ghrepo.Interface, error) {
 }
 
 func (c *fsContext) SetBaseRepo(nwo string) {
-	c.baseRepo = ghrepo.FromFullName(nwo)
+	c.baseRepo, _ = ghrepo.FromFullName(nwo)
 }
diff --git a/internal/ghrepo/repo.go b/internal/ghrepo/repo.go
@@ -28,14 +28,15 @@ func FullName(r Interface) string {
 	return fmt.Sprintf("%s/%s", r.RepoOwner(), r.RepoName())
 }
 
-// FromFullName extracts the GitHub repository inforation from an "OWNER/REPO" string
-func FromFullName(nwo string) Interface {
+// FromFullName extracts the GitHub repository information from an "OWNER/REPO" string
+func FromFullName(nwo string) (Interface, error) {
 	var r ghRepo
 	parts := strings.SplitN(nwo, "/", 2)
-	if len(parts) == 2 {
-		r.owner, r.name = parts[0], parts[1]
+	if len(parts) != 2 || parts[0] == "" || parts[1] == "" {
+		return &r, fmt.Errorf("expected OWNER/REPO format, got %q", nwo)
 	}
-	return &r
+	r.owner, r.name = parts[0], parts[1]
+	return &r, nil
 }
 
 // FromURL extracts the GitHub repository information from a URL

Original file line number	Diff line number	Diff line change
`@@ -39,7 +39,8 @@ func TestIssueList(t *testing.T) {`
`39`	`39`	`} } }`
`40`	`40`	`))
`41`	`41`
`42`		`- _, err := IssueList(client, ghrepo.FromFullName("OWNER/REPO"), "open", []string{}, "", 251, "")`
	`42`	`+ repo, _ := ghrepo.FromFullName("OWNER/REPO")`
	`43`	`+ _, err := IssueList(client, repo, "open", []string{}, "", 251, "")`
`43`	`44`	`if err != nil {`
`44`	`45`	`t.Fatalf("unexpected error: %v", err)`
`45`	`46`	`}`
Original file line number	Diff line number	Diff line change
`@@ -189,7 +189,10 @@ func repoCreate(cmd *cobra.Command, args []string) error {`
`189`	`189`	`if len(args) > 0 {`
`190`	`190`	`name = args[0]`
`191`	`191`	`if strings.Contains(name, "/") {`
`192`		`- newRepo := ghrepo.FromFullName(name)`
	`192`	`+ newRepo, err := ghrepo.FromFullName(name)`
	`193`	`+ if err != nil {`
	`194`	`+ return fmt.Errorf("argument error: %w", err)`
	`195`	`+ }`
`193`	`196`	`orgName = newRepo.RepoOwner()`
`194`	`197`	`name = newRepo.RepoName()`
`195`	`198`	`}`
`@@ -366,9 +369,9 @@ func repoFork(cmd *cobra.Command, args []string) error {`
`366`	`369`	`return fmt.Errorf("did not understand argument: %w", err)`
`367`	`370`	`}`
`368`	`371`	`} else {`
`369`		`- repoToFork = ghrepo.FromFullName(repoArg)`
`370`		`- if repoToFork.RepoName() == "" \|\| repoToFork.RepoOwner() == "" {`
`371`		`- return fmt.Errorf("could not parse owner or repo name from %s", repoArg)`
	`372`	`+ repoToFork, err = ghrepo.FromFullName(repoArg)`
	`373`	`+ if err != nil {`
	`374`	`+ return fmt.Errorf("argument error: %w", err)`
`372`	`375`	`}`
`373`	`376`	`}`
`374`	`377`	`}`
`@@ -508,7 +511,11 @@ func repoView(cmd *cobra.Command, args []string) error {`
`508`	`511`	`return fmt.Errorf("did not understand argument: %w", err)`
`509`	`512`	`}`
`510`	`513`	`} else {`
`511`		`- toView = ghrepo.FromFullName(repoArg)`
	`514`	`+ var err error`
	`515`	`+ toView, err = ghrepo.FromFullName(repoArg)`
	`516`	`+ if err != nil {`
	`517`	`+ return fmt.Errorf("argument error: %w", err)`
	`518`	`+ }`
`512`	`519`	`}`
`513`	`520`	`}`
`514`	`521`
Original file line number	Diff line number	Diff line change
`@@ -92,5 +92,6 @@ func (c *blankContext) BaseRepo() (ghrepo.Interface, error) {`
`92`	`92`	`}`
`93`	`93`
`94`	`94`	`func (c *blankContext) SetBaseRepo(nwo string) {`
`95`		`- c.baseRepo = ghrepo.FromFullName(nwo)`
	`95`	`+ repo, _ := ghrepo.FromFullName(nwo)`
	`96`	`+ c.baseRepo = repo`
`96`	`97`	`}`
Original file line number	Diff line number	Diff line change
`@@ -39,7 +39,7 @@ func ResolveRemotesToRepos(remotes Remotes, client *api.Client, base string) (Re`
`39`	`39`	`}`
`40`	`40`
`41`	`41`	`hasBaseOverride := base != ""`
`42`		`- baseOverride := ghrepo.FromFullName(base)`
	`42`	`+ baseOverride, _ := ghrepo.FromFullName(base)`
`43`	`43`	`foundBaseOverride := false`
`44`	`44`	`repos := make([]ghrepo.Interface, 0, lenRemotesForLookup)`
`45`	`45`	`for _, r := range remotes[:lenRemotesForLookup] {`
`@@ -266,5 +266,5 @@ func (c *fsContext) BaseRepo() (ghrepo.Interface, error) {`
`266`	`266`	`}`
`267`	`267`
`268`	`268`	`func (c *fsContext) SetBaseRepo(nwo string) {`
`269`		`- c.baseRepo = ghrepo.FromFullName(nwo)`
	`269`	`+ c.baseRepo, _ = ghrepo.FromFullName(nwo)`
`270`	`270`	`}`
Original file line number	Diff line number	Diff line change
`@@ -28,14 +28,15 @@ func FullName(r Interface) string {`
`28`	`28`	`return fmt.Sprintf("%s/%s", r.RepoOwner(), r.RepoName())`
`29`	`29`	`}`
`30`	`30`
`31`		`-// FromFullName extracts the GitHub repository inforation from an "OWNER/REPO" string`
`32`		`-func FromFullName(nwo string) Interface {`
	`31`	`+// FromFullName extracts the GitHub repository information from an "OWNER/REPO" string`
	`32`	`+func FromFullName(nwo string) (Interface, error) {`
`33`	`33`	`var r ghRepo`
`34`	`34`	`parts := strings.SplitN(nwo, "/", 2)`
`35`		`- if len(parts) == 2 {`
`36`		`- r.owner, r.name = parts[0], parts[1]`
	`35`	`+ if len(parts) != 2 \|\| parts[0] == "" \|\| parts[1] == "" {`
	`36`	`+ return &r, fmt.Errorf("expected OWNER/REPO format, got %q", nwo)`
`37`	`37`	`}`
`38`		`- return &r`
	`38`	`+ r.owner, r.name = parts[0], parts[1]`
	`39`	`+ return &r, nil`
`39`	`40`	`}`
`40`	`41`
`41`	`42`	`// FromURL extracts the GitHub repository information from a URL`