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

Add support for passing sort options to IssueCommentsClient.GetAllForRepository() #1501

Merged
merged 13 commits into from
Jan 7, 2017
+304 −19
Merged

Add support for passing sort options to IssueCommentsClient.GetAllForRepository() #1501

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
baabb98
Make GetAllForRepo to accept optional args
pmnxis Nov 11, 2016
fa9d3e7
Fix tests
pmnxis Nov 11, 2016
2dd5336
add missing interfaces
pmnxis Nov 16, 2016
1a7a50a
fix tests
pmnxis Nov 16, 2016
297025f
Fix tests
pmnxis Nov 16, 2016
14a11b9
fix tests
pmnxis Nov 16, 2016
cd86f24
fix tests
pmnxis Nov 21, 2016
0e13194
fix tests
pmnxis Nov 21, 2016
6d37397
fix tests
pmnxis Nov 21, 2016
ece289f
fix tests
pmnxis Nov 21, 2016
3f58504
fix
pmnxis Nov 22, 2016
7fb0768
fix tests
pmnxis Nov 23, 2016
34004ac
fix tests
pmnxis Nov 23, 2016
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
36 changes: 36 additions & 0 deletions Octokit.Reactive/Clients/IObservableIssueCommentsClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,42 @@ public interface IObservableIssueCommentsClient
/// <param name="options">Options for changing the API response</param>
IObservable<IssueComment> GetAllForRepository(long repositoryId, ApiOptions options);

/// <summary>
/// Gets Issue Comments for a repository.
/// </summary>
/// <remarks>http://developer.github.com/v3/issues/comments/#list-comments-in-a-repository</remarks>
/// <param name="owner">The owner of the repository</param>
/// <param name="name">The name of the repository</param>
/// <param name="request">The sorting <see cref="IssueCommentRequest">parameters</see></param>
IObservable<IssueComment> GetAllForRepository(string owner, string name, IssueCommentRequest request);

/// <summary>
/// Gets Issue Comments for a repository.
/// </summary>
/// <remarks>http://developer.github.com/v3/issues/comments/#list-comments-in-a-repository</remarks>
/// <param name="repositoryId">The Id of the repository</param>
/// <param name="request">The sorting <see cref="IssueCommentRequest">parameters</see></param>
IObservable<IssueComment> GetAllForRepository(long repositoryId, IssueCommentRequest request);

/// <summary>
/// Gets Issue Comments for a repository.
/// </summary>
/// <remarks>http://developer.github.com/v3/issues/comments/#list-comments-in-a-repository</remarks>
/// <param name="owner">The owner of the repository</param>
/// <param name="name">The name of the repository</param>
/// <param name="request">The sorting <see cref="IssueCommentRequest">parameters</see></param>
/// <param name="options">Options for changing the API response</param>
IObservable<IssueComment> GetAllForRepository(string owner, string name, IssueCommentRequest request, ApiOptions options);

/// <summary>
/// Gets Issue Comments for a repository.
/// </summary>
/// <remarks>http://developer.github.com/v3/issues/comments/#list-comments-in-a-repository</remarks>
/// <param name="repositoryId">The Id of the repository</param>
/// <param name="request">The sorting <see cref="IssueCommentRequest">parameters</see></param>
/// <param name="options">Options for changing the API response</param>
IObservable<IssueComment> GetAllForRepository(long repositoryId, IssueCommentRequest request, ApiOptions options);

/// <summary>
/// Gets Issue Comments for a specified Issue.
/// </summary>
Expand Down
66 changes: 64 additions & 2 deletions Octokit.Reactive/Clients/ObservableIssueCommentsClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ public IObservable<IssueComment> GetAllForRepository(string owner, string name,
Ensure.ArgumentNotNullOrEmptyString(name, "name");
Ensure.ArgumentNotNull(options, "options");

return _connection.GetAndFlattenAllPages<IssueComment>(ApiUrls.IssueComments(owner, name), null, AcceptHeaders.ReactionsPreview, options);
return GetAllForRepository(owner, name, new IssueCommentRequest(), ApiOptions.None);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be passing through the options variable rather than ApiOptions.None

}

/// <summary>
Expand All @@ -100,7 +100,69 @@ public IObservable<IssueComment> GetAllForRepository(long repositoryId, ApiOptio
{
Ensure.ArgumentNotNull(options, "options");

return _connection.GetAndFlattenAllPages<IssueComment>(ApiUrls.IssueComments(repositoryId), options);
return GetAllForRepository(repositoryId, new IssueCommentRequest(), ApiOptions.None);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be passing through the options variable rather than ApiOptions.None

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah... thanks.

}

/// <summary>
/// Gets Issue Comments for a repository.
/// </summary>
/// <remarks>http://developer.github.com/v3/issues/comments/#list-comments-in-a-repository</remarks>
/// <param name="owner">The owner of the repository</param>
/// <param name="name">The name of the repository</param>
/// <param name="request">The sorting <see cref="IssueCommentRequest">parameters</see></param>
public IObservable<IssueComment> GetAllForRepository(string owner, string name, IssueCommentRequest request)
{
Ensure.ArgumentNotNullOrEmptyString(owner, "owner");
Ensure.ArgumentNotNullOrEmptyString(name, "name");
Ensure.ArgumentNotNull(request, "request");

return GetAllForRepository(owner, name, request, ApiOptions.None);
}

/// <summary>
/// Gets Issue Comments for a repository.
/// </summary>
/// <remarks>http://developer.github.com/v3/issues/comments/#list-comments-in-a-repository</remarks>
/// <param name="repositoryId">The Id of the repository</param>
/// <param name="request">The sorting <see cref="IssueCommentRequest">parameters</see></param>
public IObservable<IssueComment> GetAllForRepository(long repositoryId, IssueCommentRequest request)
{
Ensure.ArgumentNotNull(request, "request");

return GetAllForRepository(repositoryId, request, ApiOptions.None);
}

/// <summary>
/// Gets Issue Comments for a repository.
/// </summary>
/// <remarks>http://developer.github.com/v3/issues/comments/#list-comments-in-a-repository</remarks>
/// <param name="owner">The owner of the repository</param>
/// <param name="name">The name of the repository</param>
/// <param name="request">The sorting <see cref="IssueCommentRequest">parameters</see></param>
/// <param name="options">Options for changing the API response</param>
public IObservable<IssueComment> GetAllForRepository(string owner, string name, IssueCommentRequest request, ApiOptions options)
{
Ensure.ArgumentNotNullOrEmptyString(owner, "owner");
Ensure.ArgumentNotNullOrEmptyString(name, "name");
Ensure.ArgumentNotNull(request, "request");
Ensure.ArgumentNotNull(options, "options");

return _connection.GetAndFlattenAllPages<IssueComment>(ApiUrls.IssueComments(owner, name), request.ToParametersDictionary(), AcceptHeaders.ReactionsPreview, options);
}

/// <summary>
/// Gets Issue Comments for a repository.
/// </summary>
/// <remarks>http://developer.github.com/v3/issues/comments/#list-comments-in-a-repository</remarks>
/// <param name="repositoryId">The Id of the repository</param>
/// <param name="request">The sorting <see cref="IssueCommentRequest">parameters</see></param>
/// <param name="options">Options for changing the API response</param>
public IObservable<IssueComment> GetAllForRepository(long repositoryId, IssueCommentRequest request, ApiOptions options)
{
Ensure.ArgumentNotNull(request, "request");
Ensure.ArgumentNotNull(options, "options");

return _connection.GetAndFlattenAllPages<IssueComment>(ApiUrls.IssueComments(repositoryId), request.ToParametersDictionary(), AcceptHeaders.ReactionsPreview, options);
}

/// <summary>
Expand Down
6 changes: 4 additions & 2 deletions Octokit.Tests/Clients/IssueCommentsClientTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,9 +136,11 @@ public async Task EnsuresNonNullArguments()
await Assert.ThrowsAsync<ArgumentNullException>(() => client.GetAllForRepository("owner", null));
await Assert.ThrowsAsync<ArgumentNullException>(() => client.GetAllForRepository(null, "name", ApiOptions.None));
await Assert.ThrowsAsync<ArgumentNullException>(() => client.GetAllForRepository("owner", null, ApiOptions.None));
await Assert.ThrowsAsync<ArgumentNullException>(() => client.GetAllForRepository("owner", "name", null));
await Assert.ThrowsAsync<ArgumentNullException>(() => client.GetAllForRepository("owner", "name", options: null));
await Assert.ThrowsAsync<ArgumentNullException>(() => client.GetAllForRepository("owner", "name", null, ApiOptions.None));

await Assert.ThrowsAsync<ArgumentNullException>(() => client.GetAllForRepository(1, null));
await Assert.ThrowsAsync<ArgumentNullException>(() => client.GetAllForRepository(1, options: null));
await Assert.ThrowsAsync<ArgumentNullException>(() => client.GetAllForRepository(1, null, ApiOptions.None));

await Assert.ThrowsAsync<ArgumentException>(() => client.GetAllForRepository("", "name"));
await Assert.ThrowsAsync<ArgumentException>(() => client.GetAllForRepository("owner", ""));
Expand Down
32 changes: 21 additions & 11 deletions Octokit.Tests/Reactive/ObservableIssueCommentsClientTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,8 @@ public void RequestsCorrectUrl()
client.GetAllForRepository("fake", "repo");

gitHubClient.Connection.Received(1).Get<List<IssueComment>>(
new Uri("repos/fake/repo/issues/comments", UriKind.Relative),
Args.EmptyDictionary,
new Uri("repos/fake/repo/issues/comments", UriKind.Relative),
Arg.Any<IDictionary<string, string>>(),
"application/vnd.github.squirrel-girl-preview");
}

Expand All @@ -71,7 +71,7 @@ public void RequestsCorrectUrlWithRepositoryId()
client.GetAllForRepository(1);

gitHubClient.Connection.Received(1).Get<List<IssueComment>>(
new Uri("repositories/1/issues/comments", UriKind.Relative), Args.EmptyDictionary, null);
new Uri("repositories/1/issues/comments", UriKind.Relative), Arg.Any<IDictionary<string, string>>(), null);
}

[Fact]
Expand All @@ -80,6 +80,12 @@ public void RequestsCorrectUrlWithApiOptions()
var gitHubClient = Substitute.For<IGitHubClient>();
var client = new ObservableIssueCommentsClient(gitHubClient);

var request = new IssueCommentRequest()
{
Direction = SortDirection.Ascending,
Since = DateTime.UtcNow,
Sort = PullRequestReviewCommentSort.Created
};
var options = new ApiOptions
{
StartPage = 1,
Expand All @@ -89,10 +95,7 @@ public void RequestsCorrectUrlWithApiOptions()

client.GetAllForRepository("fake", "repo", options);

gitHubClient.Connection.Received(1).Get<List<IssueComment>>(
new Uri("repos/fake/repo/issues/comments", UriKind.Relative),
Arg.Any<Dictionary<string, string>>(),
"application/vnd.github.squirrel-girl-preview");
gitHubClient.Received().Issue.Comment.GetAllForRepository("fake", "repo", request, options);
Copy link
Contributor

Choose a reason for hiding this comment

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

Id say this is the one failing, if you have a look at what the previous code was

Copy link
Contributor

Choose a reason for hiding this comment

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

These tests aren't right. They arent failing but they also aren't correct. It may be something to do with the multiple nested subclients on the end of the .Received() call

but this observable method doesnt actually call into the IGitHubClient method, it calls the _connection.GetAndFlattenPages() method and thus this test should be checking for gitHubClient.Connection.Received(1).Get....

}

[Fact]
Expand All @@ -101,6 +104,12 @@ public void RequestsCorrectUrlWithRepositoryIdWithApiOptions()
var gitHubClient = Substitute.For<IGitHubClient>();
var client = new ObservableIssueCommentsClient(gitHubClient);

var request = new IssueCommentRequest()
{
Direction = SortDirection.Ascending,
Since = DateTime.UtcNow,
Sort = PullRequestReviewCommentSort.Created
};
var options = new ApiOptions
{
StartPage = 1,
Expand All @@ -110,8 +119,7 @@ public void RequestsCorrectUrlWithRepositoryIdWithApiOptions()

client.GetAllForRepository(1, options);

gitHubClient.Connection.Received(1).Get<List<IssueComment>>(
new Uri("repositories/1/issues/comments", UriKind.Relative), Arg.Is<IDictionary<string, string>>(d => d.Count == 2), null);
gitHubClient.Received().Issue.Comment.GetAllForRepository(1, request, options);
Copy link
Contributor

Choose a reason for hiding this comment

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

and this one too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ryangribble sry, I'm not famillar with unit testing. I'll make a fix.

}

[Fact]
Expand All @@ -124,9 +132,11 @@ public async Task EnsuresNonNullArguments()
Assert.Throws<ArgumentNullException>(() => client.GetAllForRepository("owner", null));
Assert.Throws<ArgumentNullException>(() => client.GetAllForRepository(null, "name", ApiOptions.None));
Assert.Throws<ArgumentNullException>(() => client.GetAllForRepository("owner", null, ApiOptions.None));
Assert.Throws<ArgumentNullException>(() => client.GetAllForRepository("owner", "name", null));
Assert.Throws<ArgumentNullException>(() => client.GetAllForRepository("owner", "name", request: null));
Assert.Throws<ArgumentNullException>(() => client.GetAllForRepository("owner", "name", options: null));

Assert.Throws<ArgumentNullException>(() => client.GetAllForRepository(1, null));
Assert.Throws<ArgumentNullException>(() => client.GetAllForRepository(1, request: null));
Assert.Throws<ArgumentNullException>(() => client.GetAllForRepository(1, options: null));

Assert.Throws<ArgumentException>(() => client.GetAllForRepository("", "name"));
Assert.Throws<ArgumentException>(() => client.GetAllForRepository("owner", ""));
Expand Down
36 changes: 36 additions & 0 deletions Octokit/Clients/IIssueCommentsClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,42 @@ public interface IIssueCommentsClient
/// <param name="options">Options for changing the API response</param>
Task<IReadOnlyList<IssueComment>> GetAllForRepository(long repositoryId, ApiOptions options);

/// <summary>
/// Gets Issue Comments for a repository.
/// </summary>
/// <remarks>http://developer.github.com/v3/issues/comments/#list-comments-in-a-repository</remarks>
/// <param name="owner">The owner of the repository</param>
/// <param name="name">The name of the repository</param>
/// <param name="request">The sorting <see cref="IssueCommentRequest">parameters</see></param>
Task<IReadOnlyList<IssueComment>> GetAllForRepository(string owner, string name, IssueCommentRequest request);

/// <summary>
/// Gets Issue Comments for a repository.
/// </summary>
/// <remarks>http://developer.github.com/v3/issues/comments/#list-comments-in-a-repository</remarks>
/// <param name="repositoryId">The Id of the repository</param>
/// <param name="request">The sorting <see cref="IssueCommentRequest">parameters</see></param>
Task<IReadOnlyList<IssueComment>> GetAllForRepository(long repositoryId, IssueCommentRequest request);

/// <summary>
/// Gets Issue Comments for a repository.
/// </summary>
/// <remarks>http://developer.github.com/v3/issues/comments/#list-comments-in-a-repository</remarks>
/// <param name="owner">The owner of the repository</param>
/// <param name="name">The name of the repository</param>
/// <param name="request">The sorting <see cref="IssueCommentRequest">parameters</see></param>
/// <param name="options">Options for changing the API response</param>
Task<IReadOnlyList<IssueComment>> GetAllForRepository(string owner, string name, IssueCommentRequest request, ApiOptions options);

/// <summary>
/// Gets Issue Comments for a repository.
/// </summary>
/// <remarks>http://developer.github.com/v3/issues/comments/#list-comments-in-a-repository</remarks>
/// <param name="repositoryId">The Id of the repository</param>
/// <param name="request">The sorting <see cref="IssueCommentRequest">parameters</see></param>
/// <param name="options">Options for changing the API response</param>
Task<IReadOnlyList<IssueComment>> GetAllForRepository(long repositoryId, IssueCommentRequest request, ApiOptions options);

/// <summary>
/// Gets Issue Comments for a specified Issue.
/// </summary>
Expand Down
Loading