Skip to content
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.

Sign up for GitHub

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

Implement Review API for Pull Requests #1648

Merged
merged 29 commits into from
Aug 16, 2017
Merged

Implement Review API for Pull Requests #1648

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
8b34c32
First Iteration Need to finish tests and docs
hartra344 Aug 3, 2017
e2476e0
Mostly Complete
Aug 3, 2017
55d7865
Fixing tests and adding review comments
Aug 3, 2017
07f75d6
Added tests for reactive client
Aug 4, 2017
6374837
Moved Reviews inside fo the Pull request client for better organizati…
Aug 4, 2017
526fb63
Fixing bad recursive function breaking tests
Aug 4, 2017
4a2ea0b
test fixes
Aug 4, 2017
e1eced9
Add paging support to review comments call
Aug 4, 2017
1dea68d
Fixing recursive function
Aug 4, 2017
0f67d12
Addressing comments from PR
hartra344 Aug 7, 2017
83c9fb5
fixing CI break
hartra344 Aug 7, 2017
88adff2
Typo build break
hartra344 Aug 7, 2017
f3f46cd
Fixing Convention Tests
Aug 7, 2017
d852b59
Adding correct nameof() usage in Ensure
Aug 7, 2017
da004be
Small consitancy changes
Aug 7, 2017
b2abae5
Trigger build
Aug 8, 2017
214555b
Address PR Comments
hartra344 Aug 8, 2017
ac46a77
Merge remote-tracking branch 'origin/master'
hartra344 Aug 8, 2017
c6f63a6
Fixup test naming
ryangribble Aug 14, 2017
f8c9fa7
Fix sub client ordering and incorrect URL
ryangribble Aug 14, 2017
39883eb
Tidy up comments and remove StringEnum wrapper from Request models as…
ryangribble Aug 14, 2017
e30627c
Rename GetReview to Get
ryangribble Aug 14, 2017
d7952e5
tweak debugger display
ryangribble Aug 15, 2017
071ee48
Rework integration tests - implement the easy Get/GetAll ones first...
ryangribble Aug 15, 2017
f647844
Implement integration tests for Create method.
ryangribble Aug 15, 2017
418b132
Implement secondary account settings for integration tests and a new …
ryangribble Aug 15, 2017
26c5092
Add integration tests for Delete, Dismiss and Submit methods
ryangribble Aug 15, 2017
0628de8
Attempting to add comments as part of a review revealed that we cant …
ryangribble Aug 15, 2017
d4ec7c3
add second test account user/password to configure-integration-tests …
ryangribble Aug 16, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
190 changes: 190 additions & 0 deletions Octokit.Reactive/Clients/IObservablePullRequestReviewClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,190 @@
using System;
using System.Reactive;

namespace Octokit.Reactive
{
/// <summary>
/// A client for GitHub's Pull Request Review API.
/// </summary>
/// <remarks>
/// See the <a href="https://developer.github.com/v3/pulls/reviews/">Review API documentation</a> for more information.
/// </remarks>
public interface IObservablePullRequestReviewClient
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a "naming things" perspective we have a convention where we name the classes/interfaces as a plural - eg in this case IObservablePullRequestReviewsClient if you are able to rename the classes/interfaces and files?

{
/// <summary>
/// Gets reviews for a specified pull request.
/// </summary>
/// <remarks>https://developer.github.com/v3/pulls/reviews/#list-reviews-on-a-pull-request</remarks>
/// <param name="owner">The owner of the repository</param>
/// <param name="name">The name of the repository</param>
/// <param name="pullRequestId">The pull request number</param>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please name the variable number since a pull request also has an internal long Id field which we dont want to confuse here.

This comment applies to multiple files on the PR - if you can please fixup all references to pull request numbers to be called number 💓

IObservable<PullRequestReview> GetAll(string owner, string name, int pullRequestId);

/// <summary>
/// Gets reviews for a specified pull request.
/// </summary>
/// <remarks>https://developer.github.com/v3/pulls/reviews/#list-reviews-on-a-pull-request</remarks>
/// <param name="repositoryId">The Id of the repository</param>
/// <param name="pullRequestId">The pull request number</param>
IObservable<PullRequestReview> GetAll(long repositoryId, int pullRequestId);

/// <summary>
/// Gets reviews for a specified pull request.
/// </summary>
/// <remarks>https://developer.github.com/v3/pulls/reviews/#list-reviews-on-a-pull-request</remarks>
/// <param name="owner">The owner of the repository</param>
/// <param name="name">The name of the repository</param>
/// <param name="pullRequestId">The pull request number</param>
/// <param name="options">Options for changing the API response</param>
IObservable<PullRequestReview> GetAll(string owner, string name, int pullRequestId, ApiOptions options);

/// <summary>
/// Gets reviews for a specified pull request.
/// </summary>
/// <remarks>https://developer.github.com/v3/pulls/reviews/#list-reviews-on-a-pull-request</remarks>
/// <param name="repositoryId">The Id of the repository</param>
/// <param name="pullRequestId">The pull request number</param>
/// <param name="options">Options for changing the API response</param>
IObservable<PullRequestReview> GetAll(long repositoryId, int pullRequestId, ApiOptions options);

/// <summary>
/// Gets a single pull request review by ID.
/// </summary>
/// <remarks>https://developer.github.com/v3/pulls/reviews/#get-a-single-review</remarks>
/// <param name="owner">The owner of the repository</param>
/// <param name="name">The name of the repository</param>
/// <param name="pullRequestId">The pull request review comment number</param>
/// <param name="reviewId">The pull request review number</param>
IObservable<PullRequestReview> GetReview(string owner, string name, int pullRequestId, int reviewId);

/// <summary>
/// Gets a single pull request review by ID.
/// </summary>
/// <remarks>https://developer.github.com/v3/pulls/reviews/#get-a-single-review</remarks>
/// <param name="repositoryId">The Id of the repository</param>
/// <param name="pullRequestId">The pull request review comment number</param>
/// <param name="reviewId">The pull request review number</param>
IObservable<PullRequestReview> GetReview(long repositoryId, int pullRequestId, int reviewId);

/// <summary>
/// Creates a comment on a pull review.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this right? This method actually creates the pull request review (not just a comment), right?

/// </summary>
/// <remarks>https://developer.github.com/v3/pulls/reviews/#create-a-pull-request-review</remarks>
/// <param name="owner">The owner of the repository</param>
/// <param name="name">The name of the repository</param>
/// <param name="pullRequestId">The Pull Request number</param>
/// <param name="review">The review</param>
IObservable<PullRequestReview> Create(string owner, string name, int pullRequestId, PullRequestReviewCreate review);

/// <summary>
/// Creates a comment on a pull review.
/// </summary>
/// <remarks>https://developer.github.com/v3/pulls/reviews/#create-a-pull-request-review</remarks>
/// <param name="repositoryId">The Id of the repository</param>
/// <param name="pullRequestId">The Pull Request number</param>
/// <param name="review">The review</param>
IObservable<PullRequestReview> Create(long repositoryId, int pullRequestId, PullRequestReviewCreate review);


/// <summary>
/// Deletes a pull request review.
/// </summary>
/// <remarks>https://developer.github.com/v3/pulls/reviews/#delete-a-pending-review</remarks>
/// <param name="owner">The owner of the repository</param>
/// <param name="name">The name of the repository</param>
/// <param name="pullRequestId">The pull request review comment number</param>
/// <param name="reviewId">The pull request review number</param>
IObservable<Unit> Delete(string owner, string name, int pullRequestId, int reviewId);

/// <summary>
/// Deletes a pull request review.
/// </summary>
/// <remarks>https://developer.github.com/v3/pulls/reviews/#delete-a-pending-review</remarks>
/// <param name="repositoryId">The Id of the repository</param>
/// <param name="pullRequestId">The pull request review comment number</param>
/// <param name="reviewId">The pull request review number</param>
IObservable<Unit> Delete(long repositoryId, int pullRequestId, int reviewId);


/// <summary>
/// Submits an event to a pull request review.
/// </summary>
/// <remarks>https://developer.github.com/v3/pulls/reviews/#submit-a-pull-request-review</remarks>
/// <param name="owner">The owner of the repository</param>
/// <param name="name">The name of the repository</param>
/// <param name="pullRequestId">The pull request review comment number</param>
/// <param name="reviewId">The pull request review number</param>
/// <param name="submitMessage">The message and event being submitted for the review</param>
IObservable<PullRequestReview> Submit(string owner, string name, int pullRequestId, int reviewId, PullRequestReviewSubmit submitMessage);

/// <summary>
/// Submits an event to a pull request review.
/// </summary>
/// <remarks>https://developer.github.com/v3/pulls/reviews/#submit-a-pull-request-review</remarks>
/// <param name="repositoryId">The Id of the repository</param>
/// <param name="pullRequestId">The pull request review comment number</param>
/// <param name="reviewId">The pull request review number</param>
/// <param name="submitMessage">The message and event being submitted for the review</param>
IObservable<PullRequestReview> Submit(long repositoryId, int pullRequestId, int reviewId, PullRequestReviewSubmit submitMessage);

/// <summary>
/// Dismisses a pull request review.
/// </summary>
/// <remarks>https://developer.github.com/v3/pulls/reviews/#dismiss-a-pull-request-review</remarks>
/// <param name="owner">The owner of the repository</param>
/// <param name="name">The name of the repository</param>
/// <param name="pullRequestId">The pull request review comment number</param>
/// <param name="reviewId">The pull request review number</param>
/// <param name="dismissMessage">The message indicating why the review was dismissed</param>
IObservable<PullRequestReview> Dismiss(string owner, string name, int pullRequestId, int reviewId, PullRequestReviewDismiss dismissMessage);

/// <summary>
/// Dismisses a pull request review.
/// </summary>
/// <remarks>https://developer.github.com/v3/pulls/reviews/#dismiss-a-pull-request-review</remarks>
/// <param name="repositoryId">The Id of the repository</param>
/// <param name="pullRequestId">The pull request review comment number</param>
/// <param name="reviewId">The pull request review number</param>
/// <param name="dismissMessage">The message indicating why the review was dismissed</param>
IObservable<PullRequestReview> Dismiss(long repositoryId, int pullRequestId, int reviewId, PullRequestReviewDismiss dismissMessage);

/// <summary>
/// Lists comments for a single review
/// </summary>
/// <remarks>https://developer.github.com/v3/pulls/reviews/#get-comments-for-a-single-review</remarks>
/// <param name="owner">The owner of the repository</param>
/// <param name="name">The name of the repository</param>
/// <param name="pullRequestId">The pull request review comment number</param>
/// <param name="reviewId">The pull request review number</param>
IObservable<PullRequestReviewComment> GetAllComments(string owner, string name, int pullRequestId, int reviewId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will need to poke the API to see whether this GetAllComments method needs to support ApiOptions overloads for pagination

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok I was mostly going off the API docs which showed pagination support in the response
screen shot 2017-08-07 at 7 44 22 am


/// <summary>
/// Dismisses a pull request review.
/// </summary>
/// <remarks>https://developer.github.com/v3/pulls/reviews/#get-comments-for-a-single-review</remarks>
/// <param name="repositoryId">The Id of the repository</param>
/// <param name="pullRequestId">The pull request review comment number</param>
/// <param name="reviewId">The pull request review number</param>
IObservable<PullRequestReviewComment> GetAllComments(long repositoryId, int pullRequestId, int reviewId);

/// <summary>
/// Lists comments for a single review
/// </summary>
/// <remarks>https://developer.github.com/v3/pulls/reviews/#get-comments-for-a-single-review</remarks>
/// <param name="owner">The owner of the repository</param>
/// <param name="name">The name of the repository</param>
/// <param name="pullRequestId">The pull request review comment number</param>
/// <param name="reviewId">The pull request review number</param>
IObservable<PullRequestReviewComment> GetAllComments(string owner, string name, int pullRequestId, int reviewId, ApiOptions options);

/// <summary>
/// Dismisses a pull request review.
/// </summary>
/// <remarks>https://developer.github.com/v3/pulls/reviews/#get-comments-for-a-single-review</remarks>
/// <param name="repositoryId">The Id of the repository</param>
/// <param name="pullRequestId">The pull request review comment number</param>
/// <param name="reviewId">The pull request review number</param>
IObservable<PullRequestReviewComment> GetAllComments(long repositoryId, int pullRequestId, int reviewId, ApiOptions options);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove extra whitespace line

}
}
2 changes: 2 additions & 0 deletions Octokit.Reactive/Clients/IObservablePullRequestsClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ public interface IObservablePullRequestsClient
/// </summary>
IObservablePullRequestReviewRequestsClient ReviewRequest { get; }


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whitespace

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing XmlDoc comment

IObservablePullRequestReviewClient PullRequestReview { get; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are already in the PullRequestsClient class, this member should just be named Review (then it would be accessed like client.PullRequest.Review.Create()

Similar to how eg we have ReviewRequest rather than PullRequestReviewRequest

I also like to place it in the class in the same location that the API doc structure/tree indicate, so in this case it should be above ReviewComment and ReviewRequest

/// <summary>
/// Gets a single Pull Request by number.
/// </summary>
Expand Down
Loading