New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for new Pull Request Reviews preview API. #495

Closed
dmitshur opened this Issue Dec 14, 2016 · 11 comments

Comments

Projects
None yet
3 participants
@dmitshur
Member

dmitshur commented Dec 14, 2016

@sahildua2305

This comment has been minimized.

Show comment
Hide comment
@sahildua2305

sahildua2305 Dec 16, 2016

Collaborator

I'd like to work on this @shurcooL. Can you please help me in getting started?
Will it be similar to pulls_comments.go?

Collaborator

sahildua2305 commented Dec 16, 2016

I'd like to work on this @shurcooL. Can you please help me in getting started?
Will it be similar to pulls_comments.go?

@haya14busa

This comment has been minimized.

Show comment
Hide comment
@haya14busa

haya14busa Dec 16, 2016

Contributor

Hi!

@sahildua2305 I started to work on this now to use this API for my personal project, and it's in progress.
But, you commented first, so I think you can do this if you want, otherwise please let me handle it.

As for the progress, I just only implemented PullRequestReviewService.List, so progress is small.

Contributor

haya14busa commented Dec 16, 2016

Hi!

@sahildua2305 I started to work on this now to use this API for my personal project, and it's in progress.
But, you commented first, so I think you can do this if you want, otherwise please let me handle it.

As for the progress, I just only implemented PullRequestReviewService.List, so progress is small.

@sahildua2305

This comment has been minimized.

Show comment
Hide comment
@sahildua2305

sahildua2305 Dec 16, 2016

Collaborator

@haya14busa thanks for offering. I'd really like to work on this. However, I'll need help in getting started. I'm new to Go.

Collaborator

sahildua2305 commented Dec 16, 2016

@haya14busa thanks for offering. I'd really like to work on this. However, I'll need help in getting started. I'm new to Go.

@haya14busa

This comment has been minimized.

Show comment
Hide comment
@haya14busa

haya14busa Dec 16, 2016

Contributor

I think you can add rest of methods like this master...haya14busa:pull-reviews

I guess we can also use PullRequestsService instead of adding new PullRequestReviewService.

Contributor

haya14busa commented Dec 16, 2016

I think you can add rest of methods like this master...haya14busa:pull-reviews

I guess we can also use PullRequestsService instead of adding new PullRequestReviewService.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Dec 16, 2016

Member

Thanks for offering to work on this, @sahildua2305 and @haya14busa.

How about @sahildua2305 takes this and @haya14busa, you can help out with reviewing the PR and iterating on it as needed?

Can you please help me in getting started?
Will it be similar to pulls_comments.go?

I've looked at the new API. It's 6 new endpoints, and they're quite similar/related to pull requests.

I agree with @haya14busa, it looks like simply adding them to the existing PullRequestsService would be a good solution. It doesn't seem that a new service PullRequestReviewService is needed here. PR reviews are pretty similar to usual comments on PRs. It's also quite analogous to comments that issues have, and those are just a part of IssuesService.

So, let's go with adding 6 methods to PullRequestsService, similar to what's in pulls_comments.go.

For example, Get a single review endpoint can look like this (rough draft to give you a starting point):

// GetReview gets a single review for the specified pull request.
//
// 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) {
	...
}

@sahildua2305, you can look at @haya14busa's current implementation, it's quite good so far, with the exception that you want to go with the existing PullRequestsService.

Feel free to make a PR as soon as you have something started, if you'd like early feedback or to make sure you're on the right path. Thanks again!

Member

dmitshur commented Dec 16, 2016

Thanks for offering to work on this, @sahildua2305 and @haya14busa.

How about @sahildua2305 takes this and @haya14busa, you can help out with reviewing the PR and iterating on it as needed?

Can you please help me in getting started?
Will it be similar to pulls_comments.go?

I've looked at the new API. It's 6 new endpoints, and they're quite similar/related to pull requests.

I agree with @haya14busa, it looks like simply adding them to the existing PullRequestsService would be a good solution. It doesn't seem that a new service PullRequestReviewService is needed here. PR reviews are pretty similar to usual comments on PRs. It's also quite analogous to comments that issues have, and those are just a part of IssuesService.

So, let's go with adding 6 methods to PullRequestsService, similar to what's in pulls_comments.go.

For example, Get a single review endpoint can look like this (rough draft to give you a starting point):

// GetReview gets a single review for the specified pull request.
//
// 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) {
	...
}

@sahildua2305, you can look at @haya14busa's current implementation, it's quite good so far, with the exception that you want to go with the existing PullRequestsService.

Feel free to make a PR as soon as you have something started, if you'd like early feedback or to make sure you're on the right path. Thanks again!

@haya14busa

This comment has been minimized.

Show comment
Hide comment
@haya14busa

haya14busa Dec 16, 2016

Contributor

Thank you for the response, @shurcooL!

Ok, I'm happy to help @sahildua2305! Please notify me when you open a pull request or want some helps.

Contributor

haya14busa commented Dec 16, 2016

Thank you for the response, @shurcooL!

Ok, I'm happy to help @sahildua2305! Please notify me when you open a pull request or want some helps.

@sahildua2305

This comment has been minimized.

Show comment
Hide comment
@sahildua2305

sahildua2305 Dec 16, 2016

Collaborator

Thanks for your response, @shurcooL!

Also thank you for offering help, @haya14busa. I'll inform you as I open the PR soon.

Collaborator

sahildua2305 commented Dec 16, 2016

Thanks for your response, @shurcooL!

Also thank you for offering help, @haya14busa. I'll inform you as I open the PR soon.

@sahildua2305

This comment has been minimized.

Show comment
Hide comment
@sahildua2305

sahildua2305 Dec 16, 2016

Collaborator

@shurcooL @haya14busa I am just getting started with my work on this. How do I test my changes?

Collaborator

sahildua2305 commented Dec 16, 2016

@shurcooL @haya14busa I am just getting started with my work on this. How do I test my changes?

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Dec 16, 2016

Member

First, make sure all existing tests pass by running go test github.com/google/go-github/github.

You can write a simple program that exercises your new code and hits the real GitHub API, and see if it works as expected.

Then write unit tests that are similar to the ones that exist. It helps to get a real GitHub API response and make your mock handler return something equivalent.

Also see the unit test that @haya14busa wrote in ebd6006.

Member

dmitshur commented Dec 16, 2016

First, make sure all existing tests pass by running go test github.com/google/go-github/github.

You can write a simple program that exercises your new code and hits the real GitHub API, and see if it works as expected.

Then write unit tests that are similar to the ones that exist. It helps to get a real GitHub API response and make your mock handler return something equivalent.

Also see the unit test that @haya14busa wrote in ebd6006.

@sahildua2305

This comment has been minimized.

Show comment
Hide comment
@sahildua2305

sahildua2305 Dec 16, 2016

Collaborator

Thanks @shurcooL! I got everything (related to environment and existing tests) working. I will test my changes, write tests and create a PR once at least one endpoint is covered. 😄

Collaborator

sahildua2305 commented Dec 16, 2016

Thanks @shurcooL! I got everything (related to environment and existing tests) working. I will test my changes, write tests and create a PR once at least one endpoint is covered. 😄

sahildua2305 added a commit to sahildua2305/go-github that referenced this issue Dec 16, 2016

Add 'list all reviews' endpoint from new pull request review API
This commit adds only one of the total 6 endpoints made available
for developers preview. This is for initial review by other contributors. Once
reviewed, other endpoints will be added as well.

Partly fixes #495

sahildua2305 added a commit to sahildua2305/go-github that referenced this issue Dec 17, 2016

Add 'list all reviews' endpoint from new pull request review API
This commit adds only one of the total 6 endpoints made available
for developers preview.

Partly fixes #495

EDIT: Fix minor things based on initial review.
@haya14busa

This comment has been minimized.

Show comment
Hide comment
Contributor

haya14busa commented Dec 18, 2016

sahildua2305 added a commit to sahildua2305/go-github that referenced this issue Jan 5, 2017

Add 'list all reviews' endpoint from new pull request review API
This commit adds only one of the total 6 endpoints made available
for developers preview.

Partly fixes #495

EDIT: Fix minor things based on initial review.

sahildua2305 added a commit to sahildua2305/go-github that referenced this issue Jan 17, 2017

Add 'list all reviews' endpoint from new pull request review API
This commit adds only one of the total 6 endpoints made available
for developers preview.

Partly fixes #495

EDIT: Fix minor things based on initial review.

sahildua2305 added a commit to sahildua2305/go-github that referenced this issue Feb 1, 2017

Add 'list all reviews' endpoint from new pull request review API
This commit adds only one of the total 6 endpoints made available
for developers preview.

Partly fixes #495

EDIT: Fix minor things based on initial review.

@gmlewis gmlewis closed this in 27c7c32 Feb 3, 2017

BOTBrad pushed a commit to BOTBrad/go-github that referenced this issue Jun 16, 2017

Add support for new pull request review API
Fixes #495.
Closes #497.

Change-Id: If266972e7a20f83afaa842d54c22846bbb435328
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment