Add support for new pull request review API #497

sahildua2305 · 2016-12-16T23:22:15Z

fixes #495

@shurcooL @haya14busa please review. 🙂

dmitshur

This is a great start, @sahildua2305!

I've left minor comments that you should address for this endpoint, and keep them in mind for next ones.

But feel free to keep going, you're definitely on the right track. Thanks!

dmitshur · 2016-12-17T05:37:17Z

github/pulls_reviews.go

+//
+// GitHub API docs: https://developer.github.com/v3/pulls/reviews/#list-reviews-on-a-pull-request
+func (s *PullRequestsService) ListReviews(owner string, repo string, number int) ([]*PullRequestReview, *Response, error) {
+	u := fmt.Sprintf("repos/%v/%v/pulls/%v/reviews", owner, repo, number)


You can use "%d" instead of "%v" for number (but keep "%v" for owner and repo), in order to be more consistent with the rest of the code in this repository. See:

go-github/github/pulls.go

Line 212 in 466070b

u := fmt.Sprintf("repos/%v/%v/pulls/%d/files", owner, repo, number)

dmitshur · 2016-12-17T05:37:45Z

github/pulls_reviews.go

+		return nil, nil, err
+	}
+
+	req.Header.Set("Accept", mediaTypePullRequestReview)


Add a // TODO: This header will be unnecessary when the API is no longer in preview. comment above this line, for consistency.

dmitshur · 2016-12-17T05:40:30Z

github/pulls_reviews_test.go

+		t.Errorf("PullRequests.ListComments returned error: %v", err)
+	}
+
+	want := []*PullRequestReview{{ID: Int(1)}, {ID: Int(2)}}


Minor thought, but I'd make it more visible there are 2 reviews here by using multiple lines, like so:

want := []*PullRequestReview{ {ID: Int(1)}, {ID: Int(2)}, }

For example, see here.

dmitshur · 2016-12-17T05:41:05Z

github/pulls_reviews_test.go

+
+	want := []*PullRequestReview{{ID: Int(1)}, {ID: Int(2)}}
+	if !reflect.DeepEqual(reviews, want) {
+		t.Errorf("PullRequests.ListComments returned %+v, want %+v", reviews, want)


You mean PullRequests.ListReviews here, not ListComments. :)

dmitshur · 2016-12-17T05:42:17Z

github/pulls_reviews_test.go

+
+	reviews, _, err := client.PullRequests.ListReviews("o", "r", 1)
+	if err != nil {
+		t.Errorf("PullRequests.ListComments returned error: %v", err)


Also typo here, ListComments should be ListReviews.

dmitshur · 2016-12-17T05:46:08Z

github/github.go

@@ -91,6 +91,9 @@ const (

 	// https://developer.github.com/changes/2016-09-14-Integrations-Early-Access/
 	mediaTypeIntegrationPreview = "application/vnd.github.machine-man-preview+json"
+
+	// https://developer.github.com/changes/2016-12-14-reviews-api/
+	mediaTypePullRequestReview = "application/vnd.github.black-cat-preview+json"


I know it's a mouthful, but, there should be a Preview stuffix at the end. These are all preview APIs, this one happens to be named "Pull Request Reviews". So:

mediaTypePullRequestReviewsPreview = ...

haya14busa

Nice works!

haya14busa · 2016-12-17T08:44:32Z

github/pulls_reviews.go

+import (
+	"fmt"
+	"time"
+)

 // PullRequestReview represents a review of a pull request.
 type PullRequestReview struct {


https://developer.github.com/v3/pulls/reviews/#list-reviews-on-a-pull-request

Some fields of list review API response are missing.

commit_id

html_url

pull_request_url

You can see similar fields in PullRequestComment

go-github/github/pulls_comments.go

Line 14 in 7b47f69

type PullRequestComment struct {

And "state" seems UPPRECASE and can be "APPROVED", "DISMISSED", "COMMENTED".

PullRequestReview is also used in PullRequestReviewEvent https://developer.github.com/v3/activity/events/types/#pullrequestreviewevent , so maybe we should check consistencies and give feed backs to GitHub support perhaps if the differences are not reasonable.

"state" also can be "CHANGES_REQUESTED"

Sorry, I mean, "state" can be "ACCEPTED", "DISMISSED", "CHANGES_REQUESTED" or "COMMENTED" for Rest API we are working with now, but "state" also can be "approved", "rejected", or "commented" for pull-request event API. https://developer.github.com/v3/activity/events/types/#pullrequestreviewevent

PullRequestReview is used here as a field of event API.

go-github/github/event_types.go

Line 338 in 7b47f69

Review *PullRequestReview `json:"review,omitempty"`

So, it's not a good idea to remove existed comment, i guess.
Or, we can remove this comment completely and leave users to check GitHub API document, but i'm not sure.

And, there is a typo. s/ACCEPTED/APPROVED/

I will prefer removing this comment totally. @haya14busa

Agree. I think it's ok to remove it.

sahildua2305 · 2016-12-17T13:39:36Z

@shurcooL @haya14busa please review again. Made changes according to the initial feedback 😄

haya14busa

LGTM for ListReviews feature!

haya14busa · 2016-12-17T16:15:09Z

github/pulls_reviews.go

+import (
+	"fmt"
+	"time"
+)

 // PullRequestReview represents a review of a pull request.
 type PullRequestReview struct {


Sorry, I mean, "state" can be "ACCEPTED", "DISMISSED", "CHANGES_REQUESTED" or "COMMENTED" for Rest API we are working with now, but "state" also can be "approved", "rejected", or "commented" for pull-request event API. https://developer.github.com/v3/activity/events/types/#pullrequestreviewevent

PullRequestReview is used here as a field of event API.

go-github/github/event_types.go

Line 338 in 7b47f69

Review *PullRequestReview `json:"review,omitempty"`

So, it's not a good idea to remove existed comment, i guess.
Or, we can remove this comment completely and leave users to check GitHub API document, but i'm not sure.

And, there is a typo. s/ACCEPTED/APPROVED/

dmitshur · 2016-12-17T18:22:37Z

please review again. Made changes according to the initial feedback 😄

The changes look great, thanks!

One suggestion. Since you're still iterating on this PR and making progress, it's better to push additional commits without rewriting history and squashing everything into a single commit. That way, when a reviewer is reviewing new changes, it's a lot easier to see what has changed since the last time they've looked. Otherwise, the reviewer has to review the entire thing from beginning, instead of just what's changed.

Squashing during PR isn't needed because we can easily do that at the end, when merging.

sahildua2305 · 2016-12-17T18:24:24Z

@shurcooL Got it! I'll push in separate commits from now.

dmitshur

I've discovered an issue with the GitHub API documentation for 2 of the new endpoints. See inline comments.

There are 2 things we should do:

Update the code to have repo parameter, because it's needed for successful operation (confirmed by hitting real API).
It would be good to report this issue to GitHub so they fix their documentation. See the last paragraph of https://developer.github.com/changes/2016-12-14-reviews-api/:

If you have any questions or feedback, please let us know!

@sahildua2305, would you like to contact them and let them know?

dmitshur · 2016-12-17T22:30:59Z

github/pulls_reviews.go

+//
+// GitHub API docs: https://developer.github.com/v3/pulls/reviews/#get-a-single-reviews-comments
+func (s *PullRequestsService) ListReviewComments(owner string, number int, id int) ([]*PullRequestReviewComment, *Response, error) {
+	u := fmt.Sprintf("repos/%v/pulls/%d/reviews/%d/comments", owner, number, id)


This endpoint is missing the repo parameter too. We should add it, because it doesn't work otherwise.

dmitshur · 2016-12-17T22:31:11Z

github/pulls_reviews.go

+//
+// GitHub API docs: https://developer.github.com/v3/pulls/reviews/#get-a-single-review
+func (s *PullRequestsService) GetReview(owner string, number int, id int) (*PullRequestReview, *Response, error) {
+	u := fmt.Sprintf("repos/%v/pulls/%d/reviews/%d", owner, number, id)


GitHub API documentation has a mistake for this endpoint and the one below. They're both missing the repo argument.

I've tested the API, and confirmed it's a documentation issue.

For example, listing reviews with owner and repo parameters works okay:

curl -i -H "Accept: application/vnd.github.black-cat-preview+json" 'https://api.github.com/repos/google/go-github/pulls/497/reviews'

One if the IDs it returns is 13441488. Then, if we use GitHub API docs to get that single URL, without the repo parameter, it's a 404:

$ curl -i -H "Accept: application/vnd.github.black-cat-preview+json" 'https://api.github.com/repos/google/pulls/497/reviews/13441488' HTTP/1.1 404 Not Found { "message": "Not Found", "documentation_url": "https://developer.github.com/v3" }

But if we include repo (as one would expect... it makes no sense to specify a PR with just owner and PR number, without repo name), it works:

$ curl -i -H "Accept: application/vnd.github.black-cat-preview+json" 'https://api.github.com/repos/google/go-github/pulls/497/reviews/13441488' HTTP/1.1 200 OK { "id": 13441488, ... }

Thanks for noticing this @shurcooL! Even I was suspicious at the time of writing that method however, didn't delve deeper.

I have contacted GitHub about the error in their documentation. I will update the endpoint in our code.

Excellent, thank you @sahildua2305!

sahildua2305 · 2016-12-18T00:03:31Z

@shurcooL I'm not sure what to do with the event in the input for CreateReview method.

Should I add event in PullRequestReview type along with state?
How to handle comments parameter?

Same for the SubmitReview (for event parameter) and for the DismissReview (for the message parameter).

dmitshur · 2016-12-18T00:18:51Z

I'm not sure what to do with the event in the input for CreateReview method.

There are 15 structs in go-github that end with "Request". We want to use a similar pattern. There'll be a new struct for creating new PR reviews, named PullRequestReviewRequest.

For example, see the github.AuthorizationRequest struct, it's used as a parameter in AuthorizationsService.Create method to create authorizations. Or IssuesService.Create and its IssueRequest struct.

How to handle comments parameter?

comments is documented as an "array of draft review comment objects", and:

Draft review comments must include these values

Name Type Description

path string The path to the file being commented on

position integer The position in the file to be commented on

body string Body of the comment

So I'd expect a ReviewComment struct (or so) with those 3 fields, and PullRequestReviewRequest would have a Comments []ReviewComment field. Does that make sense?

sahildua2305 · 2016-12-18T00:33:33Z

@shurcooL thanks for the detailed response! I think I got it. I will implement CreateReview accordingly and then you can review. 😄

sahildua2305 · 2016-12-18T02:18:13Z

@shurcooL @haya14busa This is ready for review 🙂

haya14busa · 2016-12-18T03:46:54Z

github/pulls_reviews.go

+// ListReviewComments lists all the comments for the specified review
+//
+// GitHub API docs: https://developer.github.com/v3/pulls/reviews/#get-a-single-reviews-comments
+func (s *PullRequestsService) ListReviewComments(owner string, number int, id int) ([]*PullRequestReviewComment, *Response, error) {


Did you test this actual endpoint? It seems it doesn't work.

curl -i -H "Accept: application/vnd.github.black-cat-preview+json" 'https://api.github.com/repos/google/go-github/pulls/497/reviews/13455186/comments' { "message": "Validation Failed", "errors": [ "Cannot return null for non-nullable field PullRequestReviewComment.position" ], "documentation_url": "https://developer.github.com/v3/pulls/reviews/#get-a-single-reviews-comments" }

Maybe, we should report this to GitHub support.

Never mind. It's not go-github implementation issue. I reported to GitHub support.

What's the error you're getting?

Error:

$ curl -v -H "Accept: application/vnd.github.black-cat-preview+json" 'https://api.github.com/repos/google/go-github/pulls/497/reviews/13441488/comments' * Trying 192.30.253.117... * Connected to api.github.com (192.30.253.117) port 443 (#0) * ALPN, offering h2 * ALPN, offering http/1.1 * Cipher selection: ALL:!EXPORT:!EXPORT40:!EXPORT56:!aNULL:!LOW:!RC4:@STRENGTH * successfully set certificate verify locations: * CAfile: /etc/ssl/certs/ca-certificates.crt CApath: none * TLSv1.2 (OUT), TLS header, Certificate Status (22): * TLSv1.2 (OUT), TLS handshake, Client hello (1): * TLSv1.2 (IN), TLS handshake, Server hello (2): * TLSv1.2 (IN), TLS handshake, Certificate (11): * TLSv1.2 (IN), TLS handshake, Server key exchange (12): * TLSv1.2 (IN), TLS handshake, Server finished (14): * TLSv1.2 (OUT), TLS handshake, Client key exchange (16): * TLSv1.2 (OUT), TLS change cipher, Client hello (1): * TLSv1.2 (OUT), TLS handshake, Finished (20): * TLSv1.2 (IN), TLS change cipher, Client hello (1): * TLSv1.2 (IN), TLS handshake, Finished (20): * SSL connection using TLSv1.2 / ECDHE-RSA-AES128-GCM-SHA256 * ALPN, server accepted to use http/1.1 * Server certificate: * subject: C=US; ST=California; L=San Francisco; O=GitHub, Inc.; CN=*.github.com * start date: Apr 8 00:00:00 2014 GMT * expire date: Apr 12 12:00:00 2017 GMT * subjectAltName: host "api.github.com" matched cert's "*.github.com" * issuer: C=US; O=DigiCert Inc; OU=www.digicert.com; CN=DigiCert SHA2 High Assurance Server CA * SSL certificate verify ok. > GET /repos/google/go-github/pulls/497/reviews/13441488/comments HTTP/1.1 > Host: api.github.com > User-Agent: curl/7.50.0-DEV > Accept: application/vnd.github.black-cat-preview+json > < HTTP/1.1 422 Unprocessable Entity < Server: GitHub.com < Date: Thu, 22 Dec 2016 08:54:11 GMT < Content-Type: application/json; charset=utf-8 < Content-Length: 573 < Status: 422 Unprocessable Entity < X-RateLimit-Limit: 60 < X-RateLimit-Remaining: 20 < X-RateLimit-Reset: 1482399467 < X-GitHub-Media-Type: github.black-cat-preview; format=json < Access-Control-Expose-Headers: ETag, Link, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval < Access-Control-Allow-Origin: * < Content-Security-Policy: default-src 'none' < Strict-Transport-Security: max-age=31536000; includeSubdomains; preload < X-Content-Type-Options: nosniff < X-Frame-Options: deny < X-XSS-Protection: 1; mode=block < X-GitHub-Request-Id: 31680E9E:C366:4D04E48:585B94B2 < { "message": "Validation Failed", "errors": [ "Cannot return null for non-nullable field PullRequestReviewComment.position", "Cannot return null for non-nullable field PullRequestReviewComment.position", "Cannot return null for non-nullable field PullRequestReviewComment.position", "Cannot return null for non-nullable field PullRequestReviewComment.position", "Cannot return null for non-nullable field PullRequestReviewComment.position" ], "documentation_url": "https://developer.github.com/v3/pulls/reviews/#get-a-single-reviews-comments" }

I already reported this problem to GitHub support but it seems it's not fixed yet.

GitHub fixed this problem! ❤️

haya14busa · 2016-12-18T03:50:20Z

github/pulls_reviews.go

+// GetReview fetches the specified pull request review.
+//
+// GitHub API docs: https://developer.github.com/v3/pulls/reviews/#get-a-single-review
+func (s *PullRequestsService) GetReview(owner string, number int, id int) (*PullRequestReview, *Response, error) {


[nit] I prefer reviewID instead of id, because users cannot tell what is number and id from signature.

I noticed that even in issues_comments.go, all methods use id. I kept as id to make the code consistent. Do we want to change there as well?

haya14busa · 2016-12-18T03:58:48Z

github/pulls_reviews.go

+	return Stringify(p)
+}
+
+// PullRequestReviewComment represents a comment left on a pull request review


[nit] period missing, including other comments.

haya14busa · 2016-12-18T04:00:52Z

github/pulls_reviews.go

+	Comments []DraftReviewComment `json:"comments,omitempty"`
+
+	// Message is used (optionally) while dismissing a review
+	Message *string `json:"message,omitempty"`


Hmm... I think it's better to create another struct like PullRequestReviewDissmisalRequest or PullRequestReviewDissmisal because the format is completely different.

Yeah, I was confused between the two approaches (making a new struct and adding an optional field in PullRequestReviewRequest struct). I will make a new struct 👍

haya14busa · 2016-12-18T04:02:27Z

github/pulls_reviews.go

+type PullRequestReviewRequest struct {
+	Body     *string              `json:"body,omitempty"`
+	Event    *string              `json:"event,omitempty"`
+	Comments []DraftReviewComment `json:"comments,omitempty"`


I guess it should be Comments []*DraftReviewComment for consistency.

Are any of the fields optional? They should only be pointers with ,omitempty if they're optional. Otherwise, plain values and no ,omitempty would be more fitting.

I suggested this because I rememerbed go-github API changes from []T to []*T like ([]github.PullRequestComment -> []*github.PullRequestComment) several month ago.
The pull-request is this. #375

But, on second thought, PullRequestReviewRequest is only used as a request, not as a response.
So, I agree with you. Pointer is not needed here.

haya14busa · 2016-12-18T04:18:47Z

github/pulls_reviews.go


-	// State can be "approved", "rejected", or "commented".
+	// State can be "ACCEPTED", "DISMISSED", "CHANGES_REQUESTED" or "COMMENTED".


or "PENDING" for pending reviews.

Will add 👍

haya14busa · 2016-12-18T08:31:49Z

github/pulls_reviews.go

+type PullRequestReviewRequest struct {
+	Body     *string              `json:"body,omitempty"`
+	Event    *string              `json:"event,omitempty"`
+	Comments []DraftReviewComment `json:"comments,omitempty"`


I suggested this because I rememerbed go-github API changes from []T to []*T like ([]github.PullRequestComment -> []*github.PullRequestComment) several month ago.
The pull-request is this. #375

But, on second thought, PullRequestReviewRequest is only used as a request, not as a response.
So, I agree with you. Pointer is not needed here.

haya14busa · 2016-12-18T08:44:18Z

github/pulls_reviews.go

+
+// SubmitReview submits a specified review on the specified pull request
+//
+// GitHub API docs: https://developer.github.com/v3/pulls/reviews/#submit-a-pull-request-review


Just FYI: I reported a problem regarding this API as below. We cannot left "event" empty.

If left blank, the review will be in the PENDING state.
-- https://developer.github.com/v3/pulls/reviews/#submit-a-pull-request-review

Document says we can left "event" field for "Submit a pull request review" API as "PENDING" stete, but API doesn't support it.

Response:

{ "message": "Invalid request. "event" wasn't supplied.", "documentation_url": "https://developer.github.com/v3/pulls/reviews/#submit-a-pull-request-review" }

haya14busa · 2016-12-18T08:48:51Z

github/pulls_reviews.go

+	// TODO: remove custom Accept header when this API fully launches
+	req.Header.Set("Accept", mediaTypePullRequestReviewsPreview)
+
+	r := new(PullRequestReview)


Let's keep variable name consistent. In GetReview, The variable name is "review".
In Go, short name is preferred if it makes senses, so I think it's ok to use r, but in this case, i like "review".

Sorry, "review" is used as argument. Please ignore this.

haya14busa · 2016-12-18T09:11:32Z

As I posted request and error response example (#497 (comment)), error format for review API is not same as http://developer.github.com/v3/#client-errors

"errors" is list of string for review API.

{
  "message": "Validation Failed",
  "errors": [
    "Cannot return null for non-nullable field PullRequestReviewComment.position"
  ],
  "documentation_url": "https://developer.github.com/v3/pulls/reviews/#get-a-single-reviews-comments"
}

Corresponding go-github struct (ErrorResponse) is here.

go-github/github/github.go

Line 463 in fbc15ca

type ErrorResponse struct {

Because ErrorResponse doesn't support review API error format, error information is dropped with unmarshalling and it's really difficult to debug error using go-github.
(I tried to implement create review API in my personal project reviewdog/reviewdog#63 using go-github, but it was really hard to debug error... )

I'll report this problem to GitHub support, but this review API error fromat could be intentional.
If so, we have to handle this problem I guess.

sahildua2305 · 2016-12-21T23:32:39Z

@haya14busa I have updated the PR accordingly. Please review again. (\cc: @shurcooL)

haya14busa · 2016-12-22T08:24:11Z

github/pulls_reviews.go

+}
+
+// PullRequestReviewComment represents a comment left on a pull request review.
+type PullRequestReviewComment struct {


Please add String() method for this type as well, including other new added structs.

func (p PullRequestReviewComment) String() string { return Stringify(p) }

In addition, we can test String() method in func TestString

go-github/github/strings_test.go

Line 87 in 4dcd653

func TestString(t *testing.T) {

haya14busa · 2016-12-22T08:31:37Z

github/pulls_reviews.go

+	Body     *string `json:"body,omitempty"`
+}
+
+// PullRequestReviewRequest represents a request to create a review.


This is not go-github implementation issue, but GitHub API one, but I wonder why we cannot specify CommitID to DraftReviewComment nor PullRequestReviewRequest.

Create a Pull Request comment API support commit_id https://developer.github.com/v3/pulls/comments/#create-a-comment

When passing the commit_id, use the SHA of the latest commit in the pull request or your comment may appear as "outdated" if the specified position has been modified in a subsequent commit.

As document noted, commit_id is important to specify a line because subsequent commit may overwrite it.

It might be better to contact to GitHub support about this issue.

Did you contact GitHub about this issue? So, we will have to wait till they fix it before we can merge it?

Not, yet. Will do. (edit: sent a report)

So, we will have to wait till they fix it before we can merge it?

No, I think we don't have to wait fix(?) about this problem. We can add a field later after merging.

However, I think it's better to wait for the fix or response about the error format problem. #497 (comment)
If it's intentional, go-github should support the new error format as well.

haya14busa · 2016-12-22T08:35:08Z

github/pulls_reviews_test.go

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


I found that there are test cases for invalid request for other API.

e.g.

go-github/github/pulls_comments_test.go

Line 71 in 4dcd653

func TestPullRequestsService_ListComments_invalidOwner(t *testing.T) {

func TestPullRequestsService_ListComments_invalidOwner(t *testing.T) { _, _, err := client.PullRequests.ListComments("%", "r", 1, nil) testURLParseError(t, err) }

I think it's not so important test but it might be better to add similar tests for review API as well.

Added some of the tests, not for all new methods however.

Looks nice! Thanks 👍 8c7f0fb

Could you please adding similar tests for the rest of methods for consistency?

haya14busa · 2016-12-22T08:58:11Z

github/pulls_reviews.go

+// ListReviewComments lists all the comments for the specified review
+//
+// GitHub API docs: https://developer.github.com/v3/pulls/reviews/#get-a-single-reviews-comments
+func (s *PullRequestsService) ListReviewComments(owner string, number int, id int) ([]*PullRequestReviewComment, *Response, error) {


Error:

$ curl -v -H "Accept: application/vnd.github.black-cat-preview+json" 'https://api.github.com/repos/google/go-github/pulls/497/reviews/13441488/comments' * Trying 192.30.253.117... * Connected to api.github.com (192.30.253.117) port 443 (#0) * ALPN, offering h2 * ALPN, offering http/1.1 * Cipher selection: ALL:!EXPORT:!EXPORT40:!EXPORT56:!aNULL:!LOW:!RC4:@STRENGTH * successfully set certificate verify locations: * CAfile: /etc/ssl/certs/ca-certificates.crt CApath: none * TLSv1.2 (OUT), TLS header, Certificate Status (22): * TLSv1.2 (OUT), TLS handshake, Client hello (1): * TLSv1.2 (IN), TLS handshake, Server hello (2): * TLSv1.2 (IN), TLS handshake, Certificate (11): * TLSv1.2 (IN), TLS handshake, Server key exchange (12): * TLSv1.2 (IN), TLS handshake, Server finished (14): * TLSv1.2 (OUT), TLS handshake, Client key exchange (16): * TLSv1.2 (OUT), TLS change cipher, Client hello (1): * TLSv1.2 (OUT), TLS handshake, Finished (20): * TLSv1.2 (IN), TLS change cipher, Client hello (1): * TLSv1.2 (IN), TLS handshake, Finished (20): * SSL connection using TLSv1.2 / ECDHE-RSA-AES128-GCM-SHA256 * ALPN, server accepted to use http/1.1 * Server certificate: * subject: C=US; ST=California; L=San Francisco; O=GitHub, Inc.; CN=*.github.com * start date: Apr 8 00:00:00 2014 GMT * expire date: Apr 12 12:00:00 2017 GMT * subjectAltName: host "api.github.com" matched cert's "*.github.com" * issuer: C=US; O=DigiCert Inc; OU=www.digicert.com; CN=DigiCert SHA2 High Assurance Server CA * SSL certificate verify ok. > GET /repos/google/go-github/pulls/497/reviews/13441488/comments HTTP/1.1 > Host: api.github.com > User-Agent: curl/7.50.0-DEV > Accept: application/vnd.github.black-cat-preview+json > < HTTP/1.1 422 Unprocessable Entity < Server: GitHub.com < Date: Thu, 22 Dec 2016 08:54:11 GMT < Content-Type: application/json; charset=utf-8 < Content-Length: 573 < Status: 422 Unprocessable Entity < X-RateLimit-Limit: 60 < X-RateLimit-Remaining: 20 < X-RateLimit-Reset: 1482399467 < X-GitHub-Media-Type: github.black-cat-preview; format=json < Access-Control-Expose-Headers: ETag, Link, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval < Access-Control-Allow-Origin: * < Content-Security-Policy: default-src 'none' < Strict-Transport-Security: max-age=31536000; includeSubdomains; preload < X-Content-Type-Options: nosniff < X-Frame-Options: deny < X-XSS-Protection: 1; mode=block < X-GitHub-Request-Id: 31680E9E:C366:4D04E48:585B94B2 < { "message": "Validation Failed", "errors": [ "Cannot return null for non-nullable field PullRequestReviewComment.position", "Cannot return null for non-nullable field PullRequestReviewComment.position", "Cannot return null for non-nullable field PullRequestReviewComment.position", "Cannot return null for non-nullable field PullRequestReviewComment.position", "Cannot return null for non-nullable field PullRequestReviewComment.position" ], "documentation_url": "https://developer.github.com/v3/pulls/reviews/#get-a-single-reviews-comments" }

I already reported this problem to GitHub support but it seems it's not fixed yet.

haya14busa

Nice job! Sorry for the repetitive reviews, but I left some new comments.

I found integration tests which doesn't need GitHub API token. e.g. TestPullRequests_ListCommits
https://github.com/google/go-github/blob/4dcd653e8d6b7aa32a6d50d938cecca80a8b4b61/tests/integration/pulls_test.go

I tested some methods like this, but I guess it's awesome if you add intergration tests, for method which doens't require GitHub API token.
https://gist.github.com/haya14busa/b561b5c0df8475b34799d076253600e5#file-2016-12-22-173545-go

I think it's not strictly required to write intergration tests, but it helps a alot and worth adding it.

As for the method which requires GitHub API token, I think we have to test them at least once like the gist example I posted above (link).

Did you write something like that?

haya14busa · 2016-12-22T09:00:44Z

github/pulls_reviews.go

+// ListReviewComments lists all the comments for the specified review.
+//
+// GitHub API docs: https://developer.github.com/v3/pulls/reviews/#get-a-single-reviews-comments
+func (s *PullRequestsService) ListReviewComments(owner string, repo string, number int, reviewID int) ([]*PullRequestReviewComment, *Response, error) {


returned type should be ([]*PullRequestComment, *Response, error)

doc: https://developer.github.com/v3/pulls/reviews/#get-a-single-reviews-comments

Nice catch! Fixed

sahildua2305 · 2016-12-29T01:14:35Z

@shurcooL @haya14busa please review again.

haya14busa · 2017-01-04T15:36:23Z

Just FYI: I reported a GitHub Review API problem regarding cannceling or adding line comments to a pending review as follow, it's not go-github one but I just let you know I reported a problem regarding GitHub Review API.

Cannot cancel a pending review nor add line comments (draft review comment) to a pending review via Review API.

It seems that we cannot canncel a pending review via API nor add line comments, although we can do it via Web UI.
In addition, we cannot create more than one pending reviews.

So, once we create a pending review via Create a pull request review API,
we have no choice but to submit the review through Submit a pull request review API.

I think Reivew API should support to cancel a pending review or/and add a line comments to a pending review.

Thanks in advance.

haya14busa

LGTM! Thank you for your great work 👍

It might be better to wait for the responce from GitHub supports for some problems like this one #497 (comment) before merging this pull-request, but LGTM for now 💯

I confirmed each methods works as expected at least once. https://gist.github.com/haya14busa/6eae0c71a6dbbb652e90d227817dc8fd#file-create_review-go
ref: #497 (review)

Please fix the problem I described in this comment https://github.com/google/go-github/pull/497/files#r92924211 and fix the merge conflict.

haya14busa · 2017-01-04T14:41:38Z

github/pulls_reviews.go

+	return r, resp, err
+}
+
+// SubmitReview submits a specified review on the specified pull request.


Just FYI: I reported a GitHub Review API problem as follow, it's not go-github one but I just let you know I reported a problem regarding this API.

Cannot use Submit a pull request review for public repositories with API token which has only public_repo as a scope.
It seems the API token needs repo scope (Full control of private repositories).
I confirmed this behavior by toggling the scope of the API token.

With API token which only has public_repo as a scope, "Submit a pull request review" API returns NotFound, API token which has repo scope works as expected.

Not Found error:

{ "message": "Not Found", "documentation_url": "https://developer.github.com/v3/pulls/reviews/#submit-a-pull-request-review" }

On the other hands, Create a pull request review works as expected with API token which only has public_repo as a scope.

"Submit a pull request review" API should work with API token which only has public_repo.

Thanks in advance.

Thanks for reporting it to GitHub support @haya14busa and looking into it.

If I'm reading this right, it sounds like the problem is a purely permissions one and is on their backend. If they resolve it by changing the permission requirements, then there wouldn't be any API changes on our side, would there?

If so, I would suggest we lean towards not letting this issue block the PR from being merged.

If so, I would suggest we lean towards not letting this issue block the PR from being merged.

Agree. I just let you know to share the problem not to make you get into the same problem.

However, as for the different problem I described in this comment #497 (comment) , it's might be better to wait the response because it's related go-github implementation.
It will be really hard to get error messages via go-github at current implementation.

But i'm not sure. We can also merge p-r and fix it later. I leave it to you.
GitHub support kindly contacted me several times, but they didn't fix it yet.
They may overlook this problem because I reported several other problems.

May I ping them about the problem?

Pinging after some time should be fine. If they're already on it, they'll let you know.

So, while we wait for them to respond/fix the issue, should we go ahead with this PR and land it? It's been hanging around for a while now 😛

I'm okay with landing this sooner as long as the affected endpoints have the problem documented and a TODO comment saying what needs to be followed up with.

But if the problem is serious enough to affect all endpoints, we may want to hold a bit more.

I'll ping them a few hours later. We want to know the error format is expected or not at least.

Thanks, @haya14busa. Let us know once you hear back from them 😃

@haya14busa did you get any update?

GitHub support tells me that he contacted to developer teams to share the problem, but no document or implementation fix yet.

sahildua2305 · 2017-01-05T23:38:23Z

@haya14busa thanks for green signal 💚

@shurcooL please check 😃

dmitshur

I've looked over this and I did not spot any issues. LGTM.

Huge thanks @sahildua2305 and @haya14busa for working on this!

From what I can see, the only blocking issue before merging this is the potentially misbehaving SubmitReview endpoint, is that right?

I left a comment about it. My recommendation would be to try to get this PR merged sooner rather than wait for a complete resolution there. Reasons being:

it sounds like the problem resolution (if any) would not have significant/any effect on the API
this is an preview API, meaning it's not considered stable yet. there may be additional problems on github backend. our responsibility in go-github is to map the Go methods to the correct endpoints and structs.
merging this sooner means more people can start trying out and beta-testing these new endpoints, potentially uncovering problems sooner
it's less work to merge sooner than later

GitHub usually has very quick turnaround, so I expect it'll take somewhere between a day~a week. But I'd rather us not be blocked unnecessarily.

The only thing we should do to make the PR merge-able now is to document the known issue with the SubmitReview endpoint and add a TODO comment to follow it up. Then it can safely be merged sooner.

sahildua2305 · 2017-01-06T01:35:05Z

Thanks for the review @shurcooL :)

Where do we add the corresponding TODO as you mentioned in the review?
Also, do we add preview API endpoints to this spreadsheet? If so, please add. :)

haya14busa · 2017-01-06T01:38:09Z

The only thing we should do to make the PR merge-able now is to document the known issue with the SubmitReview endpoint

No. As I said in this comment, the only thing we should care about is problem about error response format.
#497 (comment)

I think we can ignore other problems because they are not related with go-github implementation.

- Change return type of ListReviewComments method to PullRequestComment. - Corresponding changes in unit tests. - Remove type PullRequestReviewComment. - Remove string test for PullRequestReviewComment.

- Added new API endpoint for deleting a pending review. - Wrote unit tests for the same.

haya14busa

Also, since it's been one week, did you hear back from GitHub support?

GitHub Supports said that they doesn't share public timeline to fix the problem nor tell me the direction to fix it (document fix or implementation fix).
They will get in touch with me once there are some progress.

I looked at the document about error format https://developer.github.com/v3/#client-errors again, but there are no document fix yet at least right now.

I assume (or I should say I hope) they are going to fix the problem by implementation fix, because documentation fix must be easy and it doen't take lots of time. But, your guess is as good as mine.

I think it's ok to merge this p-r, but could you please adding comments to each method about this problem?

haya14busa · 2017-02-01T11:28:58Z

github/pulls_reviews.go

+// Note: There is a known issue with this preview endpoint.
+// Read more about it here - https://github.com/google/go-github/pull/497#discussion_r94593877
+//
+// TODO: Remove this once we arrive at resolution with GitHub Support.


This should be fixed now. Can you confirm and remove this?

haya14busa · 2017-02-01T11:44:31Z

github/pulls_reviews.go

+// DeletePendingReview deletes the specified pull request pending review.
+//
+// GitHub API docs: https://developer.github.com/v3/pulls/reviews/#delete-a-pending-review
+func (s *PullRequestsService) DeletePendingReview(owner string, repo string, number int, reviewID int) (*PullRequestReview, *Response, error) {


gmlewis

This is looking great, @sahildua2305!

I have just a few minor tweaks... and please be aware that these are not mistakes you have made... I went ahead and changed some things out from under you in the repo while you were working on this.
If you don't wish to address them, that is fine too, and I can make the changes in a separate PR... your call.

Thank you, @shurcooL and @haya14busa for your continued reviews on this PR... they are greatly appreciated!

gmlewis · 2017-02-01T16:52:12Z

github/pulls_reviews.go

+type PullRequestReviewRequest struct {
+	Body     *string              `json:"body,omitempty"`
+	Event    *string              `json:"event,omitempty"`
+	Comments []DraftReviewComment `json:"comments,omitempty"`


Please use slice of pointers, not slice of values:

Comments []*DraftReviewComment `json:"comments,omitempty"`

gmlewis · 2017-02-01T16:52:56Z

github/pulls_reviews.go

+// ListReviews lists all reviews on the specified pull request.
+//
+// GitHub API docs: https://developer.github.com/v3/pulls/reviews/#list-reviews-on-a-pull-request
+func (s *PullRequestsService) ListReviews(owner string, repo string, number int) ([]*PullRequestReview, *Response, error) {


(owner, repo string, number int)

gmlewis · 2017-02-01T16:54:09Z

github/pulls_reviews.go

+	// TODO: remove custom Accept header when this API fully launches
+	req.Header.Set("Accept", mediaTypePullRequestReviewsPreview)
+
+	reviews := new([]*PullRequestReview)


var reviews []*PullRequestReview resp, err := s.client.Do(req, &reviews) if err != nil { return nil, resp, err } return reviews, resp, nil

gmlewis · 2017-02-01T16:54:34Z

github/pulls_reviews.go

+// GetReview fetches the specified pull request review.
+//
+// GitHub API docs: https://developer.github.com/v3/pulls/reviews/#get-a-single-review
+func (s *PullRequestsService) GetReview(owner string, repo string, number int, reviewID int) (*PullRequestReview, *Response, error) {


(owner, repo string, number, reviewID int)

gmlewis · 2017-02-01T16:55:03Z

github/pulls_reviews.go

+		return nil, resp, err
+	}
+
+	return review, resp, err


return review, resp, nil

gmlewis · 2017-02-01T16:59:41Z

github/pulls_reviews.go

+// TODO: Remove this once we arrive at resolution with GitHub Support.
+//
+// GitHub API docs: https://developer.github.com/v3/pulls/reviews/#submit-a-pull-request-review
+func (s *PullRequestsService) SubmitReview(owner string, repo string, number int, reviewID int, review *PullRequestReviewRequest) (*PullRequestReview, *Response, error) {


(owner, repo string, number, reviewID int, review *PullRequestReviewRequest)

gmlewis · 2017-02-01T16:59:54Z

github/pulls_reviews.go

+		return nil, resp, err
+	}
+
+	return r, resp, err


return r, resp, nil

gmlewis · 2017-02-01T17:00:19Z

github/pulls_reviews.go

+// DismissReview dismisses a specified review on the specified pull request.
+//
+// GitHub API docs: https://developer.github.com/v3/pulls/reviews/#dismiss-a-pull-request-review
+func (s *PullRequestsService) DismissReview(owner string, repo string, number int, reviewID int, review *PullRequestReviewDismissalRequest) (*PullRequestReview, *Response, error) {


(owner, repo string, number, reviewID int, review *PullRequestReviewDismissalRequest)

gmlewis · 2017-02-01T17:03:58Z

github/pulls_reviews.go

+}
+
+// PullRequestReviewDismissalRequest represents a request to dismiss a review.
+type PullRequestReviewDismissalRequest struct {


I wish we could reduce the stutter on this type... it reminds me too much of Java. 😃

One thing I can think of is to use PRReviewDismissalRequest but that will disturb the consistency. 😞

Agreed. I couldn't think of a better alternative. I was just lamenting. 😄

gmlewis · 2017-02-01T17:04:22Z

github/pulls_reviews.go


-	// State can be "approved", "rejected", or "commented".
-	State *string `json:"state,omitempty"`
+	return r, resp, err


return r, resp, nil

- Includes majorly the changes of return err to return nil when err is nil.

sahildua2305 · 2017-02-01T23:24:16Z

@gmlewis thanks for the review. I went ahead and fixed all the minor issues. Please review again 😄

gmlewis

LGTM. Thank you, @sahildua2305!
I'll wait for @shurcooL to LGTM before merging in case I've provided feedback that he doesn't agree with.

haya14busa · 2017-02-01T23:31:32Z

@sahildua2305

I think it's ok to merge this p-r, but could you please adding comments to each method about this problem?
#497 (review)

Could you please adding comments for known issues as well?

dmitshur · 2017-02-01T23:40:15Z

Still LGTM, I don't have change requests from myself. However, I think the point @haya14busa is making should be addressed (it's just a documentation change).

sahildua2305 · 2017-02-01T23:44:07Z

@haya14busa according to our previous discussion, all endpoints in this PR have the aforementioned issue except list reviews and create review. Is that right?

haya14busa · 2017-02-01T23:51:22Z

except list reviews and create review.

No. Create review endpoint also has the same problem.
I didn't confirm all endpoints by hand, but all review API returns the same error format.
You can check error of create review by passing wrong line number or something.

You might be able to skip GET endpoints because we cannot reproduce the problem again(GitHub fixed some problems for those endpoints), but just to be on the safe side, I think it's better to note the problem for each endpoints.

sahildua2305 · 2017-02-02T00:12:07Z

@haya14busa done 😄

haya14busa · 2017-02-02T00:16:28Z

Thank you for the fix @sahildua2305 👍 👍 👍
LGTM

dmitshur · 2017-02-02T05:34:24Z

github/pulls_reviews.go

+// TODO: Follow up with GitHub support about an issue with this method's
+// returned error format and remove this comment once it's fixed.
+// Read mroe about it here -
+// https://github.com/google/go-github/pull/497#issuecomment-267810912


There's a typo here, "mroe" should be "more".

Instead of linking to the middle of this PR with 100+ of comments, let's use a new issue to track this and point people at it. This will be much friendlier.

I've made #540 - please link to it instead.

sahildua2305 · 2017-02-02T21:03:30Z

@shurcooL Done! Fixed 😄

gmlewis

LGTM. Thank you, @sahildua2305!

gmlewis · 2017-02-03T00:01:02Z

Merging.

gmlewis · 2017-02-03T00:06:33Z

Integrated in 27c7c32

Fixes google#495. Closes google#497. Change-Id: If266972e7a20f83afaa842d54c22846bbb435328

dmitshur reviewed Dec 17, 2016

View reviewed changes

haya14busa suggested changes Dec 17, 2016

View reviewed changes

sahildua2305 force-pushed the add-pullrequest-review branch from 7b47f69 to 38a474f Compare December 17, 2016 13:36

haya14busa reviewed Dec 17, 2016

View reviewed changes

dmitshur reviewed Dec 17, 2016

View reviewed changes

sahildua2305 force-pushed the add-pullrequest-review branch 2 times, most recently from 03eeede to fbc15ca Compare December 18, 2016 02:16

haya14busa reviewed Dec 18, 2016

View reviewed changes

haya14busa reviewed Dec 22, 2016

View reviewed changes

haya14busa suggested changes Dec 22, 2016

View reviewed changes

sahildua2305 changed the title ~~WIP: Add support for new pull request review API~~ Add support for new pull request review API Dec 29, 2016

haya14busa approved these changes Jan 4, 2017

View reviewed changes

sahildua2305 force-pushed the add-pullrequest-review branch from 1ad2f71 to 0d9faa8 Compare January 5, 2017 23:29

dmitshur approved these changes Jan 6, 2017

View reviewed changes

sahildua2305 added 4 commits February 1, 2017 02:06

Replace PullRequestReviewComment with PullRequestComment

a176b03

- Change return type of ListReviewComments method to PullRequestComment. - Corresponding changes in unit tests. - Remove type PullRequestReviewComment. - Remove string test for PullRequestReviewComment.

Remove misleading comment from PullRequestReview

e3b5e69

Add a note and todo about known issue with SubmitReview

a1bf055

Add DeletePendingReview endpoint

91cc8ae

- Added new API endpoint for deleting a pending review. - Wrote unit tests for the same.

sahildua2305 force-pushed the add-pullrequest-review branch from 48e9d90 to 91cc8ae Compare February 1, 2017 01:07

haya14busa reviewed Feb 1, 2017

View reviewed changes

gmlewis requested changes Feb 1, 2017

View reviewed changes

Make minor changes as suggested by gmlewis

bfda8ba

- Includes majorly the changes of return err to return nil when err is nil.

gmlewis approved these changes Feb 1, 2017

View reviewed changes

gmlewis added the lgtm label Feb 1, 2017

Doc: Add comment about known error format issue

36dbd69

dmitshur mentioned this pull request Feb 2, 2017

Pull Request Reviews (Preview API) methods have unusual error format that is not parsed. #540

Closed

dmitshur requested changes Feb 2, 2017

View reviewed changes

Fix typo and mention issue link as comment

0c0faa4

dmitshur approved these changes Feb 2, 2017

View reviewed changes

gmlewis approved these changes Feb 3, 2017

View reviewed changes

gmlewis closed this in 27c7c32 Feb 3, 2017

sahildua2305 deleted the add-pullrequest-review branch February 3, 2017 00:25

bubg-dev pushed a commit to bubg-dev/go-github that referenced this pull request Jun 16, 2017

Add support for new pull request review API

6f9dc9a

Fixes google#495. Closes google#497. Change-Id: If266972e7a20f83afaa842d54c22846bbb435328


		// State can be "approved", "rejected", or "commented".
		// State can be "ACCEPTED", "DISMISSED", "CHANGES_REQUESTED" or "COMMENTED".

Add support for new pull request review API #497

Add support for new pull request review API #497

Conversation

sahildua2305 commented Dec 16, 2016 • edited Loading

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

haya14busa left a comment

Choose a reason for hiding this comment

haya14busa Dec 17, 2016 • 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

sahildua2305 commented Dec 17, 2016

haya14busa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitshur commented Dec 17, 2016 • edited Loading

sahildua2305 commented Dec 17, 2016 • edited Loading

dmitshur left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitshur Dec 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sahildua2305 commented Dec 18, 2016 • edited Loading

dmitshur commented Dec 18, 2016

sahildua2305 commented Dec 18, 2016

sahildua2305 commented Dec 18, 2016

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

haya14busa commented Dec 18, 2016 • edited Loading

sahildua2305 commented Dec 21, 2016

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

haya14busa Dec 24, 2016 • 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

haya14busa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sahildua2305 commented Dec 29, 2016 • edited Loading

haya14busa commented Jan 4, 2017

haya14busa 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

sahildua2305 commented Dec 16, 2016 •

edited

Loading

haya14busa Dec 17, 2016 •

edited

Loading

dmitshur commented Dec 17, 2016 •

edited

Loading

sahildua2305 commented Dec 17, 2016 •

edited

Loading

dmitshur left a comment •

edited

Loading

dmitshur Dec 17, 2016 •

edited

Loading

sahildua2305 commented Dec 18, 2016 •

edited

Loading

haya14busa commented Dec 18, 2016 •

edited

Loading

haya14busa Dec 24, 2016 •

edited

Loading

sahildua2305 commented Dec 29, 2016 •

edited

Loading

dmitshur Jan 6, 2017 •

edited

Loading

sahildua2305 commented Jan 5, 2017 •

edited

Loading

dmitshur left a comment •

edited

Loading