Repository navigation
Add Preview Support for Team Review Requests - #682
Conversation
Yes, please do. That'll help us communicate that there are breaking API changes being done (as a result of GitHub preview API changing). |
| @@ -13,18 +13,22 @@ import ( | |||
| // RequestReviewers creates a review request for the provided GitHub users for the specified pull request. | |||
There was a problem hiding this comment.
for the provided GitHub users
This was true before the change, but we'll need to update docs now to describe the new API.
| // | ||
| // 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) { | ||
| func (s *PullRequestsService) RequestReviewers(ctx context.Context, owner, repo string, number int, logins []string, teams []string) (*PullRequest, *Response, error) { |
There was a problem hiding this comment.
@gmlewis, what do you think about this API decision?
An alternative to consider (that I can think of) is to create a ReviewersRequest struct which contains the two string slices, and have the caller pass that in.
I'm pretty torn between the two, it seems they tradeoffs are pretty much equivalent and I'm not strongly in favor of either of them.
|
@RaviTezu Thanks for your work on this PR! It's looking very solid so far. I've only briefly looked at it. First, I want to think about the API and decide which direction we'll want to go. Afterwards, I can review the implementation details more closely. |
…so, update the docs to reflect the changes
|
LGTM |
|
@shurcooL Any thoughts on how to move forward with this API? |
dmitshur
left a comment
dmitshur
left a comment
There was a problem hiding this comment.
I've thought about this for a while, and it's not easy to do this, but I will make the suggest to consider changing the API after all.
This is my alternative suggestion:
// ReviewersRequest specifies users and teams for a pull request review request.
type ReviewersRequest struct {
Reviewers []string `json:"reviewers,omitempty"`
TeamReviewers []string `json:"team_reviewers,omitempty"`
}
func (s *PullRequestsService) RequestReviewers(ctx context.Context, owner, repo string, number int, reviewers ReviewersRequest) (*PullRequest, *Response, error) { ... }
func (s *PullRequestsService) RemoveReviewers(ctx context.Context, owner, repo string, number int, reviewers ReviewersRequest) (*Response, error) { ... }There are 2 main reasons for this suggestion:
-
I think it might lead to more readable usage code. Consider:
pull, _, err := c.PullRequests.RequestReviewers(ctx, "o", "r", 1, []string{"a", "b"}, nil) pull, _, err := c.PullRequests.RequestReviewers(ctx, "o", "r", 1, nil, []string{"c", "d"}) pull, _, err := c.PullRequests.RequestReviewers(ctx, "o", "r", 1, []string{"a", "b"}, []string{"c", "d"})
With code like that, it's harder to see what the parameters are. Compare with:
pull, _, err := c.PullRequests.RequestReviewers(ctx, "o", "r", 1, github.ReviewersRequest{ Reviewers: []string{"a", "b"}, TeamReviewers: nil, }) pull, _, err := c.PullRequests.RequestReviewers(ctx, "o", "r", 1, github.ReviewersRequest{ Reviewers: nil, TeamReviewers: []string{"c", "d"}, }) pull, _, err := c.PullRequests.RequestReviewers(ctx, "o", "r", 1, github.ReviewersRequest{ Reviewers: []string{"a", "b"}, TeamReviewers: []string{"c", "d"}, })
-
I'm worried there's a small chance of additional types of reviewers that the GitHub API might get added in the future. If so, we would have to break the API yet again, and make it even more unreadable by having 3 slices in a row.
I don't think the difference between the two options is significant at this time. That makes this decision harder. But, given the options and information we have now, since we're making a breaking API change, I have a slight preference for this proposed API in the long run.
What do you think?
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
| // TODO: remove custom Accept header when this API fully launches. |
There was a problem hiding this comment.
For consistency with other places, add a blank line above this line.
|
@shurcooL's suggestions SGTM. |
dmitshur
left a comment
dmitshur
left a comment
There was a problem hiding this comment.
LGTM. Thanks for doing this!
One minor comment, afterwards we can merge.
| type ReviewersRequest struct { | ||
| Reviewers []string `json:"reviewers,omitempty"` | ||
| TeamReviewers []string `json:"team_reviewers,omitempty"` | ||
| } |
There was a problem hiding this comment.
Minor, swap the order of these two structs, so ReviewersRequest, the input, is on top, and Reviewers, the output, is below it.
… and Revewiers struct is used for output
This is a breaking API change to support an upcoming github.com REST API breaking change. It adds support to add, delete, and fetch team review requests. GitHub Announcement: https://developer.github.com/changes/2017-07-26-team-review-request-thor-preview/ Increase libraryVersion by one. Fixes google#678.
This is a breaking API change to support an upcoming github.com REST API breaking change. It adds support to add, delete, and fetch team review requests. GitHub Announcement: https://developer.github.com/changes/2017-07-26-team-review-request-thor-preview/ Increase libraryVersion by one. Fixes google#678.
Adding preview support for team review requests.
Fixes #678