Add support for passing sort options to IssueCommentsClient.GetAllForRepository() #1501
Conversation
|
Hi @pjc0247 sorry for the delay in responding. The builds are currently failing unit test let me know if you need help with it |
| 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); |
There was a problem hiding this comment.
Id say this is the one failing, if you have a look at what the previous code was
There was a problem hiding this comment.
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....
| @@ -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); | |||
There was a problem hiding this comment.
This should be passing through the options variable rather than ApiOptions.None
| @@ -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); | |||
There was a problem hiding this comment.
This should be passing through the options variable rather than ApiOptions.None
| 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); |
There was a problem hiding this comment.
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....
|
|
||
| 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); |
There was a problem hiding this comment.
@ryangribble sry, I'm not famillar with unit testing. I'll make a fix.
|
@ryangribble finally understood. they already have 2 parameters which contain pagenation data.... |
|
Sorry for the delay, this looks good to merge now! Many thanks for the contribution, and for your patience :) |
ryangribble
changed the title
ryangribble
changed the title
nickfloyd
added
Type: Feature
Original Issue : #1500
Hi,
I added 4 methods to
IssueCommentsClient.cs.But there are two duplicated options which named
IssueCommentSortandPullRequestReviewCommentSort. I think these types can be merged like aSortDirectionoption.Thanks.