Clean up method return consistency (#2318)

github/actions_artifacts_test.go

@@ -25,7 +25,7 @@ func TestActionsService_ListArtifacts(t *testing.T) {
 		testFormValues(t, r, values{"page": "2"})
 		fmt.Fprint(w,
 			`{
-				"total_count":1, 
+				"total_count":1,
 				"artifacts":[{"id":1}]
 			}`,
 		)
@@ -107,7 +107,7 @@ func TestActionsService_ListWorkflowRunArtifacts(t *testing.T) {
 		testFormValues(t, r, values{"page": "2"})
 		fmt.Fprint(w,
 			`{
-				"total_count":1, 
+				"total_count":1,
 				"artifacts":[{"id":1}]
 			}`,
 		)

github/activity_notifications_test.go

@@ -59,7 +59,7 @@ func TestActivityService_ListNotification(t *testing.T) {
 	})
 }
 
-func TestActivityService_ListRepositoryNotification(t *testing.T) {
+func TestActivityService_ListRepositoryNotifications(t *testing.T) {
 	client, mux, _, teardown := setup()
 	defer teardown()
 
@@ -81,7 +81,7 @@ func TestActivityService_ListRepositoryNotification(t *testing.T) {
 
 	const methodName = "ListRepositoryNotifications"
 	testBadOptions(t, methodName, func() (err error) {
-		_, _, err = client.Activity.ListRepositoryNotifications(ctx, "\n", "\n", nil)
+		_, _, err = client.Activity.ListRepositoryNotifications(ctx, "\n", "\n", &NotificationListOptions{})
 		return err
 	})
 

github/activity_star.go

@@ -108,6 +108,7 @@ func (s *ActivityService) IsStarred(ctx context.Context, owner, repo string) (bo
 	if err != nil {
 		return false, nil, err
 	}
+
 	resp, err := s.client.Do(ctx, req, nil)
 	starred, err := parseBoolResponse(err)
 	return starred, resp, err
@@ -122,6 +123,7 @@ func (s *ActivityService) Star(ctx context.Context, owner, repo string) (*Respon
 	if err != nil {
 		return nil, err
 	}
+
 	return s.client.Do(ctx, req, nil)
 }
 
@@ -134,5 +136,6 @@ func (s *ActivityService) Unstar(ctx context.Context, owner, repo string) (*Resp
 	if err != nil {
 		return nil, err
 	}
+
 	return s.client.Do(ctx, req, nil)
 }

github/billing.go

@@ -79,7 +79,7 @@ func (s *BillingService) GetActionsBillingOrg(ctx context.Context, org string) (
 		return nil, resp, err
 	}
 
-	return actionsOrgBilling, resp, err
+	return actionsOrgBilling, resp, nil
 }
 
 // GetPackagesBillingOrg returns the free and paid storage used for GitHub Packages in gigabytes for an Org.
@@ -98,7 +98,7 @@ func (s *BillingService) GetPackagesBillingOrg(ctx context.Context, org string)
 		return nil, resp, err
 	}
 
-	return packagesOrgBilling, resp, err
+	return packagesOrgBilling, resp, nil
 }
 
 // GetStorageBillingOrg returns the estimated paid and estimated total storage used for GitHub Actions
@@ -118,7 +118,7 @@ func (s *BillingService) GetStorageBillingOrg(ctx context.Context, org string) (
 		return nil, resp, err
 	}
 
-	return storageOrgBilling, resp, err
+	return storageOrgBilling, resp, nil
 }
 
 // GetAdvancedSecurityActiveCommittersOrg returns the GitHub Advanced Security active committers for an organization per repository.
@@ -137,7 +137,7 @@ func (s *BillingService) GetAdvancedSecurityActiveCommittersOrg(ctx context.Cont
 		return nil, resp, err
 	}
 
-	return activeOrgCommitters, resp, err
+	return activeOrgCommitters, resp, nil
 }
 
 // GetActionsBillingUser returns the summary of the free and paid GitHub Actions minutes used for a user.
@@ -156,7 +156,7 @@ func (s *BillingService) GetActionsBillingUser(ctx context.Context, user string)
 		return nil, resp, err
 	}
 
-	return actionsUserBilling, resp, err
+	return actionsUserBilling, resp, nil
 }
 
 // GetPackagesBillingUser returns the free and paid storage used for GitHub Packages in gigabytes for a user.
@@ -175,7 +175,7 @@ func (s *BillingService) GetPackagesBillingUser(ctx context.Context, user string
 		return nil, resp, err
 	}
 
-	return packagesUserBilling, resp, err
+	return packagesUserBilling, resp, nil
 }
 
 // GetStorageBillingUser returns the estimated paid and estimated total storage used for GitHub Actions
@@ -195,5 +195,5 @@ func (s *BillingService) GetStorageBillingUser(ctx context.Context, user string)
 		return nil, resp, err
 	}
 
-	return storageUserBilling, resp, err
+	return storageUserBilling, resp, nil
 }

github/gists.go

@@ -279,6 +279,7 @@ func (s *GistsService) Delete(ctx context.Context, id string) (*Response, error)
 	if err != nil {
 		return nil, err
 	}
+
 	return s.client.Do(ctx, req, nil)
 }
 
@@ -291,6 +292,7 @@ func (s *GistsService) Star(ctx context.Context, id string) (*Response, error) {
 	if err != nil {
 		return nil, err
 	}
+
 	return s.client.Do(ctx, req, nil)
 }
 
@@ -303,6 +305,7 @@ func (s *GistsService) Unstar(ctx context.Context, id string) (*Response, error)
 	if err != nil {
 		return nil, err
 	}
+
 	return s.client.Do(ctx, req, nil)
 }
 
@@ -315,6 +318,7 @@ func (s *GistsService) IsStarred(ctx context.Context, id string) (bool, *Respons
 	if err != nil {
 		return false, nil, err
 	}
+
 	resp, err := s.client.Do(ctx, req, nil)
 	starred, err := parseBoolResponse(err)
 	return starred, resp, err

github/git_blobs.go

@@ -33,7 +33,11 @@ func (s *GitService) GetBlob(ctx context.Context, owner string, repo string, sha
 
 	blob := new(Blob)
 	resp, err := s.client.Do(ctx, req, blob)
-	return blob, resp, err
+	if err != nil {
+		return nil, resp, err
+	}
+
+	return blob, resp, nil
 }
 
 // GetBlobRaw fetches a blob's contents from a repo.
@@ -46,11 +50,16 @@ func (s *GitService) GetBlobRaw(ctx context.Context, owner, repo, sha string) ([
 	if err != nil {
 		return nil, nil, err
 	}
+
 	req.Header.Set("Accept", "application/vnd.github.v3.raw")
 
 	var buf bytes.Buffer
 	resp, err := s.client.Do(ctx, req, &buf)
-	return buf.Bytes(), resp, err
+	if err != nil {
+		return nil, resp, err
+	}
+
+	return buf.Bytes(), resp, nil
 }
 
 // CreateBlob creates a blob object.
@@ -65,5 +74,9 @@ func (s *GitService) CreateBlob(ctx context.Context, owner string, repo string,
 
 	t := new(Blob)
 	resp, err := s.client.Do(ctx, req, t)
-	return t, resp, err
+	if err != nil {
+		return nil, resp, err
+	}
+
+	return t, resp, nil
 }

github/git_blobs_test.go

@@ -49,6 +49,14 @@ func TestGitService_GetBlob(t *testing.T) {
 		_, _, err = client.Git.GetBlob(ctx, "\n", "\n", "\n")
 		return err
 	})
+
+	testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
+		got, resp, err := client.Git.GetBlob(ctx, "o", "r", "s")
+		if got != nil {
+			t.Errorf("testNewRequestAndDoFailure %v = %#v, want nil", methodName, got)
+		}
+		return resp, err
+	})
 }
 
 func TestGitService_GetBlob_invalidOwner(t *testing.T) {
@@ -144,6 +152,14 @@ func TestGitService_CreateBlob(t *testing.T) {
 		_, _, err = client.Git.CreateBlob(ctx, "\n", "\n", input)
 		return err
 	})
+
+	testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
+		got, resp, err := client.Git.CreateBlob(ctx, "o", "r", input)
+		if got != nil {
+			t.Errorf("testNewRequestAndDoFailure %v = %#v, want nil", methodName, got)
+		}
+		return resp, err
+	})
 }
 
 func TestGitService_CreateBlob_invalidOwner(t *testing.T) {

github/git_commits_test.go

@@ -285,7 +285,18 @@ func TestGitService_CreateSignedCommitWithInvalidParams(t *testing.T) {
 	ctx := context.Background()
 	_, _, err := client.Git.CreateCommit(ctx, "o", "r", input)
 	if err == nil {
-		t.Errorf("Expected error to be returned because invalid params was passed")
+		t.Errorf("Expected error to be returned because invalid params were passed")
+	}
+}
+
+func TestGitService_CreateSignedCommitWithNilCommit(t *testing.T) {
+	client, _, _, teardown := setup()
+	defer teardown()
+
+	ctx := context.Background()
+	_, _, err := client.Git.CreateCommit(ctx, "o", "r", nil)
+	if err == nil {
+		t.Errorf("Expected error to be returned because commit=nil")
 	}
 }
 

github/git_tags.go

@@ -45,7 +45,11 @@ func (s *GitService) GetTag(ctx context.Context, owner string, repo string, sha
 
 	tag := new(Tag)
 	resp, err := s.client.Do(ctx, req, tag)
-	return tag, resp, err
+	if err != nil {
+		return nil, resp, err
+	}
+
+	return tag, resp, nil
 }
 
 // CreateTag creates a tag object.
@@ -72,5 +76,9 @@ func (s *GitService) CreateTag(ctx context.Context, owner string, repo string, t
 
 	t := new(Tag)
 	resp, err := s.client.Do(ctx, req, t)
-	return t, resp, err
+	if err != nil {
+		return nil, resp, err
+	}
+
+	return t, resp, nil
 }

github/git_tags_test.go

@@ -40,6 +40,14 @@ func TestGitService_GetTag(t *testing.T) {
 		_, _, err = client.Git.GetTag(ctx, "\n", "\n", "\n")
 		return err
 	})
+
+	testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
+		got, resp, err := client.Git.GetTag(ctx, "o", "r", "s")
+		if got != nil {
+			t.Errorf("testNewRequestAndDoFailure %v = %#v, want nil", methodName, got)
+		}
+		return resp, err
+	})
 }
 
 func TestGitService_CreateTag(t *testing.T) {
@@ -61,10 +69,11 @@ func TestGitService_CreateTag(t *testing.T) {
 	})
 
 	ctx := context.Background()
-	tag, _, err := client.Git.CreateTag(ctx, "o", "r", &Tag{
+	inputTag := &Tag{
 		Tag:    input.Tag,
 		Object: &GitObject{SHA: input.Object},
-	})
+	}
+	tag, _, err := client.Git.CreateTag(ctx, "o", "r", inputTag)
 	if err != nil {
 		t.Errorf("Git.CreateTag returned error: %v", err)
 	}
@@ -76,12 +85,17 @@ func TestGitService_CreateTag(t *testing.T) {
 
 	const methodName = "CreateTag"
 	testBadOptions(t, methodName, func() (err error) {
-		_, _, err = client.Git.CreateTag(ctx, "\n", "\n", &Tag{
-			Tag:    input.Tag,
-			Object: &GitObject{SHA: input.Object},
-		})
+		_, _, err = client.Git.CreateTag(ctx, "\n", "\n", inputTag)
 		return err
 	})
+
+	testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
+		got, resp, err := client.Git.CreateTag(ctx, "o", "r", inputTag)
+		if got != nil {
+			t.Errorf("testNewRequestAndDoFailure %v = %#v, want nil", methodName, got)
+		}
+		return resp, err
+	})
 }
 
 func TestTag_Marshal(t *testing.T) {

github/github.go

@@ -353,6 +353,7 @@ func NewEnterpriseClient(baseURL, uploadURL string, httpClient *http.Client) (*C
 	if err != nil {
 		return nil, err
 	}
+
 	if !strings.HasSuffix(baseEndpoint.Path, "/") {
 		baseEndpoint.Path += "/"
 	}
@@ -366,6 +367,7 @@ func NewEnterpriseClient(baseURL, uploadURL string, httpClient *http.Client) (*C
 	if err != nil {
 		return nil, err
 	}
+
 	if !strings.HasSuffix(uploadEndpoint.Path, "/") {
 		uploadEndpoint.Path += "/"
 	}
@@ -390,6 +392,7 @@ func (c *Client) NewRequest(method, urlStr string, body interface{}) (*http.Requ
 	if !strings.HasSuffix(c.BaseURL.Path, "/") {
 		return nil, fmt.Errorf("BaseURL must have a trailing slash, but %q does not", c.BaseURL)
 	}
+
 	u, err := c.BaseURL.Parse(urlStr)
 	if err != nil {
 		return nil, err
@@ -437,6 +440,7 @@ func (c *Client) NewUploadRequest(urlStr string, reader io.Reader, size int64, m
 	if err != nil {
 		return nil, err
 	}
+
 	req.ContentLength = size
 
 	if mediaType == "" {
@@ -621,6 +625,7 @@ func (c *Client) BareDo(ctx context.Context, req *http.Request) (*Response, erro
 	if ctx == nil {
 		return nil, errNonNilContext
 	}
+
 	req = withContext(ctx, req)
 
 	rateLimitCategory := category(req.URL.Path)
@@ -996,6 +1001,7 @@ func CheckResponse(r *http.Response) error {
 	if c := r.StatusCode; 200 <= c && c <= 299 {
 		return nil
 	}
+
 	errorResponse := &ErrorResponse{Response: r}
 	data, err := ioutil.ReadAll(r.Body)
 	if err == nil && data != nil {

github/github_test.go

@@ -594,6 +594,12 @@ func TestNewUploadRequest_badURL(t *testing.T) {
 	c := NewClient(nil)
 	_, err := c.NewUploadRequest(":", nil, 0, "")
 	testURLParseError(t, err)
+
+	const methodName = "NewUploadRequest"
+	testBadOptions(t, methodName, func() (err error) {
+		_, err = c.NewUploadRequest("\n", nil, -1, "\n")
+		return err
+	})
 }
 
 func TestNewUploadRequest_errorForNoTrailingSlash(t *testing.T) {