Add requestreviewers #557

sevki · 2017-02-18T19:58:01Z

this commit adds the RequestReviewers function as described here

https://developer.github.com/v3/pulls/review_requests/#create-a-review-request

btw: sorry I'm new to the codebase and followed the CONTRIBUTING to the best
of my ability, if you have any fixes I'll do my best to address them

Resolves #516.

this commit adds the RequestReviewers function as described here https://developer.github.com/v3/pulls/review_requests/#create-a-review-request

dmitshur · 2017-02-21T22:47:40Z

@sevki, is this meant to resolve (partially or in full) #516?

sevki · 2017-02-22T18:41:55Z

@shurcooL complete but I wanted to get something clear before I did the other two (list, delete).
I am not sure about the naming, it's not analogous to the GitHub API because PullRequestReviewRequest was already a thing. I think the request there, correct me if I'm wrong, refers to the HTTP request. and PullRequestReviewRequestRequest sounded very Java-like. So I wanted to avoid that. Are you ok with the patch as is?

dmitshur · 2017-02-23T03:11:17Z

PullRequestReviewRequest was already a thing. I think the request there, correct me if I'm wrong, refers to the HTTP request. and PullRequestReviewRequestRequest sounded very Java-like.

We have a pattern where some structs have Request or Options suffix. Usually, Request suffix is used for methods that create new things. It's a request to create that thing. And options are usually used for other methods, like options for listing. E.g.:

// IssueRequest represents a request to create/edit an issue.
// It is separate from Issue above because otherwise Labels
// and Assignee fail to serialize to the correct JSON.
type IssueRequest struct {

// MilestoneListOptions specifies the optional parameters to the
// IssuesService.ListMilestones method.
type MilestoneListOptions struct

So, in the case of PullRequestReviewRequest:

// PullRequestReviewRequest represents a request to create a review.
type PullRequestReviewRequest struct {

It's a request to create a PR review. It's a parameter for CreateReview and SubmitReview methods.

From what I see in https://developer.github.com/v3/pulls/review_requests/, there are 3 operations that can be done with "review requests":

List
Create
Delete

But review requests are basically github usernames. Both Create and Delete operations contain nothing more than an array of user logins. List operation returns an array of users.

I think we can get away without creating any structs for these 3 methods. That would be simpler.

How about something like this?

// ListReviewers lists users whose reviews have been requested on the specified pull request.
//
// GitHub API docs: https://developer.github.com/v3/pulls/review_requests/#list-review-requests
func (s *PullRequestsService) ListReviewers(owner, repo string, number int) (*[]User, *Response, error) {
	...
}

// RequestReviewers creates a review request for the provided GitHub users for the specified pull request.
//
// GitHub API docs: https://developer.github.com/v3/pulls/review_requests/#create-a-review-request
func (s *PullRequestsService) RequestReviewers(owner, repo string, number int, reviewers []string) (*PullRequest, *Response, error) {
	...
}

// RemoveReviewers removes the review request for the provided GitHub users for the specified pull request.
//
// GitHub API docs: https://developer.github.com/v3/pulls/review_requests/#delete-a-review-request
func (s *PullRequestsService) RemoveReviewers(owner, repo string, number int, reviewers []string) (*Response, error) {
	...
}

It is inspired by similar operations that can be done on issue assignees. See https://github.com/google/go-github/blob/master/github/issues_assignees.go.

varadarajana · 2017-02-25T18:14:09Z

@sevki in your PR createRequest is done, please tell me if I can take up list and Delete methods or you have already finished them.

sevki · 2017-02-25T18:15:02Z

@varadarajana ah yeah they are, sorry forgot to push them.

varadarajana · 2017-02-26T05:54:28Z

@sevki The build is failing here.

github/pulls_reviews_request.go:79: not enough arguments in call to s.client.Do
The command "go test -v -race ./..." exited with 2.

I think it might be due to context parameter added recently, can you please check if you have the latest version from the mainline.

…iewers

dmitshur

Thanks @sevki. The actual implementation is looking good, but I have some comments on improving the tests.

dmitshur · 2017-02-27T21:25:03Z

github/pulls_reviewers_test.go

+	want := []User{
+		{
+			Login: &_login,
+			ID:    &_id,


Go style doesn't use underscores in identifier names, see https://github.com/golang/go/wiki/CodeReviewComments#mixed-caps.

Also, these variables are not needed. This can be replaced with:

{ Login: String("octocat"), ID: Int(1), }

dmitshur · 2017-02-27T21:25:52Z

github/pulls_reviewers_test.go

+	})
+
+	// This returns a PR, unmarshalling of which is tested elsewhere
+	have, _, err := client.PullRequests.ListReviewers(context.Background(), "o", "r", 1)


Rename have to reviewers for consistency with other tests.

Alternatively, use the word got rather than have. Some other tests use got, but no one uses have.

dmitshur · 2017-02-27T21:26:45Z

github/pulls_reviewers_test.go

+		fmt.Fprintf(w, sampleResponse)
+	})
+
+	// This returns a PR, unmarshalling of which is tested elsewhere


I don't understand this comment. ListReviewers returns a []*User, not *PullRequest. Is there a mistake here?

dmitshur · 2017-02-27T21:27:26Z

github/pulls_reviewers_test.go

+			ID:    &_id,
+		},
+	}
+	if !reflect.DeepEqual(have, &want) {


Would it work without taking address of want, i.e.:

if !reflect.DeepEqual(have, want) {

Or would that fail? Other tests don't seem to take address of want usually.

it breaks the tests.

What does the error message say?

Never mind, I see that it's because ListReviewers returns *[]User and not []User. Your new version looks good.

@shurcooL I looked at a couple of things and saw they return pointers to arrays, is that not the consistent thing to do? if not I can change it

dmitshur · 2017-02-27T21:31:02Z

github/pulls_reviewers_test.go

+		Reviewers: logins,
+	}
+	if !reflect.DeepEqual(have, want) {
+		t.Errorf("PullRequests.ListReviews returned %+v, want %+v", have, want)


This is misleading an not accurate. It says "PullRequests.ListReviews", but the method that was called was PullRequests.RemoveReviewers".

I think you should move this check directly inside mux.HandleFunc.

👍 Looks like I mixed Remove and Request 😃

dmitshur · 2017-02-27T21:31:34Z

github/pulls_reviewers_test.go

+	type reviewers struct {
+		Reviewers []string `json:"reviewers,omitempty"`
+	}
+	have := reviewers{}


Don't use have here, see detailed comments below.

dmitshur · 2017-02-27T21:31:41Z

github/pulls_reviewers_test.go

+	type reviewers struct {
+		Reviewers []string `json:"reviewers,omitempty"`
+	}
+	have := reviewers{}


Don't use have here either, see comments below.

dmitshur · 2017-02-27T21:32:54Z

github/pulls_reviewers.go

+	"fmt"
+)
+
+// RequestReviewers submits a set of logins to be potential reviewers on a PR.


I think it'd be nicer to stay closer to the language the GitHub API docs use and say that this method creates a "review request". What do you think of what I wrote earlier?

// RequestReviewers creates a review request for the provided GitHub users for the specified pull request.

That seems more consistent with the other endpoints (like the remove one).

- changed have to reviewers, to do that I have declared a reviewersRequest type but it's scope is limited to the test package - checking requests are now performed inside the mux functions

dmitshur

Looking good. Some more minor suggestions, and I think that's all I have.

dmitshur · 2017-02-28T00:32:39Z

github/pulls_reviewers_test.go

+	setup()
+	defer teardown()
+
+	sampleResponse := `[


You can inline this, it's used in one place. Also, make it a single line, it's short enough to be readable:

[{"login": "octocat", "id": 1}]

You said 👍 here, but you didn't apply this change. Did you miss it?

I mean the part about "make it a single line, it's short enough to be readable".

dmitshur · 2017-02-28T00:35:54Z

github/pulls_reviewers_test.go

+		}
+		if !reflect.DeepEqual(reviewers, want) {
+			t.Errorf("PullRequests.RemoveReviewers returned %+v, want %+v", reviewers, want)
+		}


I think you can replace all of the above, starting with b, err := ioutil.ReadAll(r.Body), with:

testBody(t, r, `{"reviewers":["octocat","googlebot"]}`)

dmitshur · 2017-02-28T00:36:12Z

github/pulls_reviewers_test.go

+func TestRemoveReviewers(t *testing.T) {
+	setup()
+	defer teardown()
+	logins := []string{"octocat", "googlebot"}


Inline logins, it's used in one place.

dmitshur · 2017-02-28T00:37:18Z

github/pulls_reviewers_test.go

+		}
+		if !reflect.DeepEqual(reviewers, want) {
+			t.Errorf("PullRequests.RequestReviewers returned %+v, want %+v", reviewers, want)
+		}


I think you can replace all of the above, starting with b, err := ioutil.ReadAll(r.Body), with:

testBody(t, r, `{"reviewers":["octocat","googlebot"]}`)

dmitshur · 2017-02-28T00:37:29Z

github/pulls_reviewers_test.go

+	setup()
+	defer teardown()
+
+	logins := []string{"octocat", "googlebot"}


Inline logins, it's used in one place.

dmitshur · 2017-02-28T00:38:19Z

github/pulls_reviewers_test.go

+
+type reviewersRequest struct {
+	Reviewers []string `json:"reviewers,omitempty"`
+}


If you apply my suggestions below, this won't be needed, so remove it.

dmitshur · 2017-02-28T00:40:26Z

github/pulls_reviewers_test.go

+		}
+	})
+
+	// This returns a PR, unmarshalling of which is tested elsewhere


It's not hard to test it. You don't need to test all fields. Just include the PR number.

In the response handler above, add this at the end:

fmt.Fprint(w, `{"number":1}`)

And then do:

pull, _, err := client.PullRequests.RequestReviewers(...) ... want := &PullRequest{Number: Int(1)} if !reflect.DeepEqual(pull, want) { t.Errorf("PullRequests.RequestReviewers returned %+v, want %+v", pull, want) }

dmitshur · 2017-02-28T00:40:59Z

github/pulls_reviewers_test.go

+	if err != nil {
+		t.Errorf("PullRequests.RequestReviewers returned error: %v", err)
+	}
+


Nitpick, remove the unnecessary and inconsistent blank line.

dmitshur · 2017-02-28T00:42:17Z

github/pulls_reviewers_test.go

+	mux.HandleFunc("/repos/o/r/pulls/1/requested_reviewers", func(w http.ResponseWriter, r *http.Request) {
+		testMethod(t, r, "GET")
+		testHeader(t, r, "Accept", mediaTypePullRequestReviewsPreview)
+		fmt.Fprintf(w, sampleResponse)


Don't use fmt.Fprintf where fmt.Fprint will suffice. You don't want the extra formatting options if you're not using them, they can only cause unwanted issues.

dmitshur · 2017-02-28T00:43:25Z

github/pulls_reviewers.go

+// GitHub API docs: https://developer.github.com/v3/pulls/review_requests/#create-a-review-request
+func (s *PullRequestsService) RequestReviewers(ctx context.Context, owner, repo string, number int, logins []string) (*PullRequest, *Response, error) {
+	u := fmt.Sprintf("repos/%s/%s/pulls/%d/requested_reviewers", owner, repo, number)
+	reviewers := struct {


Nitpick, RemoveReviewers has a blank line between u and reviewers, but this doesn't. Please make it consistent.

- use testBody() - inline logins - fix formatting

dmitshur

I think we're getting to the end of all issues I was able to spot. Just 3 more minor typos.

dmitshur · 2017-02-28T19:42:38Z

github/pulls_reviewers_test.go

+		},
+	}
+	if !reflect.DeepEqual(reviewers, want) {
+		t.Errorf("PullRequests.ListReviews returned %+v, want %+v", reviewers, want)


s/PullRequests.ListReviews/PullRequests.ListReviewers

dmitshur · 2017-02-28T19:43:02Z

github/pulls_reviewers_test.go

+
+	_, err := client.PullRequests.RemoveReviewers(context.Background(), "o", "r", 1, []string{"octocat", "googlebot"})
+	if err != nil {
+		t.Errorf("PullRequests.RequestReviewers returned error: %v", err)


s/PullRequests.RequestReviewers/PullRequests.RemoveReviewers

dmitshur · 2017-02-28T19:44:42Z

github/pulls_reviewers_test.go

+
+	reviewers, _, err := client.PullRequests.ListReviewers(context.Background(), "o", "r", 1)
+	if err != nil {
+		t.Errorf("PullRequests.ListReviewers error: %v", err)


For consitency with lines 34 and 50 (and elsewhere in the project), insert the word "returned":

t.Errorf("PullRequests.ListReviewers returned error: %v", err)

sevki · 2017-02-28T19:47:09Z

@shurcooL so the latest changes are going to wail because testBody() don't seem to trim whitespace. the request body seems to end with a new line but the inlined JSON doesn't. Is this an issue you are aware of or am I doing something wrong?

dmitshur · 2017-02-28T19:48:24Z

@shurcooL so the latest changes are going to wail because testBody() don't seem to trim whitespace. the request body seems to end with a new line but the inlined JSON doesn't. Is this an issue you are aware of or am I doing something wrong?

That's fine, just add a newline to expected JSON output. See here for example:

go-github/github/activity_notifications_test.go

Line 78 in 9be6ce1

testBody(t, r, `{"last_read_at":"2006-01-02T15:04:05Z"}`+"\n")

sevki · 2017-02-28T19:50:04Z

can I send a PR to trim whitespace when reading the testBody?

dmitshur · 2017-02-28T19:50:19Z

Also, @sevki, please prefer to push new commits rather than amending previous ones and force pushing. New commits make it easier to review just the changes you've done. Thanks!

dmitshur · 2017-02-28T19:50:57Z

can I send a PR to trim whitespace when reading the testBody?

I'd rather we don't do that, I think it's better to be explicit. If we trim whitespace in testBody, it wouldn't catch an issue where we accidentally send too much unwanted whitespace.

It's easy enough to explicitly include a newline in the expected JSON string.

- fix some typos - add new line to test body

sevki · 2017-02-28T19:55:00Z

cool

dmitshur · 2017-02-28T19:57:25Z

github/pulls_reviewers_test.go

@@ -58,12 +58,12 @@ func TestListReviewers(t *testing.T) {
 	mux.HandleFunc("/repos/o/r/pulls/1/requested_reviewers", func(w http.ResponseWriter, r *http.Request) {
 		testMethod(t, r, "GET")
 		testHeader(t, r, "Accept", mediaTypePullRequestReviewsPreview)
-		fmt.Fprint(w, `[{"login": "octocat","id": 1 }]`)
+		fmt.Fprint(w, `[{"login": "octocat","id":1}]`)


Is there a reason you decided to keep the space between "login": and "octocat", but not between "id": and 1?

not using monospace fonts so I missed it.

dmitshur

Thanks @sevki.

I'm not spotting any other issues.

I've edited your PR description to add "Resolves #516.", since this PR will now fully resolve that issue.

@gmlewis, can you review the API we've gone with here? It's not a direct 1:1 mapping to the names that GitHub documentation uses, because we tried to make it a cleaner API without the need for a PullRequestReviewRequestRequest struct. How does it look to you?

sevki · 2017-02-28T20:10:49Z

@shurcooL thanks for the review.

gmlewis

Apart from the requested changes, I like it.
Thank you, @sevki and @shurcooL!

gmlewis · 2017-02-28T22:00:11Z

github/pulls_reviewers.go

+// ListReviewers lists users whose reviews have been requested on the specified pull request.
+//
+// GitHub API docs: https://developer.github.com/v3/pulls/review_requests/#list-review-requests
+func (s *PullRequestsService) ListReviewers(ctx context.Context, owner, repo string, number int) (*[]User, *Response, error) {


There is no need to return a pointer to a slice. Just return the slice which is already a reference type.

...([]User, *Response, error) {

@gmlewis, per https://godoc.org/github.com/google/go-github/github#UsersService.ListAll and #180 (comment), shouldn't we go with []*User?

Wow! Yes! Where was my brain?

gmlewis · 2017-02-28T22:00:29Z

github/pulls_reviewers.go

+		return nil, resp, err
+	}
+
+	return &users, resp, nil


return users, resp, nil

gmlewis · 2017-02-28T22:04:05Z

github/pulls_reviewers_test.go

+		t.Errorf("PullRequests.ListReviewers returned error: %v", err)
+	}
+
+	want := &[]User{


want := []User{

Based on #557 (comment), this should become:

want := []*User{

dmitshur · 2017-03-01T18:49:21Z

github/pulls_reviewers_test.go

-	want := &[]User{
-		{
+	want := []*User{
+		&User{


It's not neccessary to include &User{ here, just { as before will do.

In fact, it causes Travis to fail because we check that gofmt -s doesn't find any missed opportunities to simplify:

The rest of this change is perfect.

sevki · 2017-03-05T09:50:45Z

@shurcooL done.

dmitshur · 2017-03-05T15:11:32Z

@gmlewis PTAL.

gmlewis

Thank you, @sevki and @shurcooL!
LGTM.

sevki · 2017-03-06T22:31:10Z

@gmlewis my pleasure. @shurcooL did all the heavy lifting 😄

dmitshur · 2017-03-08T00:57:41Z

Thanks @sevki!

Add support for an addition to the Pull Request Reviews API, the ability to manage Review Requests. GitHub announcement: https://developer.github.com/changes/2016-12-16-review-requests-api/ GitHub API docs: https://developer.github.com/v3/pulls/review_requests/ Resolves google#516.

sevki added 2 commits February 18, 2017 20:53

Add requestreviewers

c940114

this commit adds the RequestReviewers function as described here https://developer.github.com/v3/pulls/review_requests/#create-a-review-request

add missing new line

eead948

dmitshur mentioned this pull request Feb 21, 2017

Add support for Preview Review Requests API #516

Closed

request reviewers list and delete functions

3d66e9c

sevki added 7 commits February 26, 2017 11:16

Merge branch 'master' of https://github.com/google/go-github into rev…

817dd53

…iewers

add ctx

d122ebf

add request reviewers tests

3977482

pulls_reviews_request -> pulls_reviewers

df46fa8

add missing new line at the end

e9ab56b

simplify code according to gofmt

4a5d897

fix comments and formatting

e1d9534

dmitshur requested changes Feb 27, 2017

View reviewed changes

fix PR issues

7540355

- changed have to reviewers, to do that I have declared a reviewersRequest type but it's scope is limited to the test package - checking requests are now performed inside the mux functions

dmitshur requested changes Feb 28, 2017

View reviewed changes

sevki force-pushed the reviewers branch from b268d01 to 316e2cc Compare February 28, 2017 19:15

another round of PR fixes

56eee95

- use testBody() - inline logins - fix formatting

sevki force-pushed the reviewers branch from 316e2cc to 56eee95 Compare February 28, 2017 19:44

dmitshur reviewed Feb 28, 2017

View reviewed changes

fix PR issues

4f9f8e3

- fix some typos - add new line to test body

dmitshur reviewed Feb 28, 2017

View reviewed changes

make spaces consistent

b74bcf8

dmitshur approved these changes Feb 28, 2017

View reviewed changes

gmlewis requested changes Feb 28, 2017

View reviewed changes

fix pointer issues

08a3813

dmitshur reviewed Mar 1, 2017

View reviewed changes

gofmt simplyify

9ec6c3d

gmlewis approved these changes Mar 5, 2017

View reviewed changes

dmitshur merged commit cfbfe91 into google:master Mar 8, 2017

Add requestreviewers #557

Add requestreviewers #557

Conversation

sevki commented Feb 18, 2017 • edited by dmitshur Loading

dmitshur commented Feb 21, 2017

sevki commented Feb 22, 2017

dmitshur commented Feb 23, 2017

varadarajana commented Feb 25, 2017

sevki commented Feb 25, 2017

varadarajana commented Feb 26, 2017

dmitshur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitshur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitshur Feb 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitshur Feb 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitshur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sevki commented Feb 28, 2017

dmitshur commented Feb 28, 2017

sevki commented Feb 28, 2017

dmitshur commented Feb 28, 2017 • edited Loading

dmitshur commented Feb 28, 2017 • edited Loading

sevki commented Feb 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitshur left a comment

Choose a reason for hiding this comment

sevki commented Feb 28, 2017

gmlewis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitshur Mar 1, 2017 • edited Loading

sevki commented Feb 18, 2017 •

edited by dmitshur

Loading

dmitshur Feb 28, 2017 •

edited

Loading

dmitshur Feb 28, 2017 •

edited

Loading

dmitshur commented Feb 28, 2017 •

edited

Loading

dmitshur commented Feb 28, 2017 •

edited

Loading

dmitshur Mar 1, 2017 •

edited

Loading

dmitshur Mar 1, 2017 •

edited

Loading