Add pagination support for PullRequest ops #198
Conversation
|
The test failures don't look related to the changes (all related to https://travis-ci.org/47deg/github4s/jobs/349562816#L1343-L1417 |
|
@lloydmeta thanks for your contribution. Forked PRs will always fail because there are some integration tests that use a test token, only available from the original repository. Please, try to execute your tests locally. I'll execute all the tests locally to verify that everything is ok. |
There was a problem hiding this comment.
@lloydmeta I've executed the tests locally and it works flawlessly. Could you please add a new test in GHPullRequestsSpec passing a pagination? Thanks!
Left some comments also
| @@ -27,7 +27,7 @@ trait GHPullRequestsSpec[T] extends BaseIntegrationSpec[T] { | |||
| "PullRequests >> List" should "return a right response when valid repo is provided" in { | |||
| val response = | |||
| Github(accessToken).pullRequests | |||
| .list(validRepoOwner, validRepoName) | |||
| .list(validRepoOwner, validRepoName, pagination = None) | |||
There was a problem hiding this comment.
None is the default value, right? Can we let this as it was before?
-- .list(validRepoOwner, validRepoName, pagination = None)
++ .list(validRepoOwner, validRepoName)| @@ -41,7 +41,8 @@ trait GHPullRequestsSpec[T] extends BaseIntegrationSpec[T] { | |||
| .list( | |||
| validRepoOwner, | |||
| validRepoName, | |||
| List(PRFilterAll, PRFilterSortCreated, PRFilterOrderAsc)) | |||
| List(PRFilterAll, PRFilterSortCreated, PRFilterOrderAsc), | |||
| None) | |||
| @@ -53,7 +54,7 @@ trait GHPullRequestsSpec[T] extends BaseIntegrationSpec[T] { | |||
| it should "return error when an invalid repo name is passed" in { | |||
| val response = | |||
| Github(accessToken).pullRequests | |||
| .list(validRepoOwner, invalidRepoName) | |||
| .list(validRepoOwner, invalidRepoName, pagination = None) | |||
| @@ -62,7 +63,7 @@ trait GHPullRequestsSpec[T] extends BaseIntegrationSpec[T] { | |||
| "PullRequests >> ListFiles" should "return a right response when a valid repo is provided" in { | |||
| val response = | |||
| Github(accessToken).pullRequests | |||
| .listFiles(validRepoOwner, validRepoName, validPullRequestNumber) | |||
| .listFiles(validRepoOwner, validRepoName, validPullRequestNumber, None) | |||
| @@ -74,7 +75,7 @@ trait GHPullRequestsSpec[T] extends BaseIntegrationSpec[T] { | |||
| "PullRequests >> ListFiles" should "return a right response when a valid repo is provided and not all files have 'patch'" in { | |||
| val response = | |||
| Github(None).pullRequests | |||
| .listFiles("scala", "scala", 4877) | |||
| .listFiles("scala", "scala", 4877, None) | |||
| @@ -86,7 +87,7 @@ trait GHPullRequestsSpec[T] extends BaseIntegrationSpec[T] { | |||
| it should "return error when an invalid repo name is passed" in { | |||
| val response = | |||
| Github(accessToken).pullRequests | |||
| .listFiles(validRepoOwner, invalidRepoName, validPullRequestNumber) | |||
| .listFiles(validRepoOwner, invalidRepoName, validPullRequestNumber, None) | |||
| @@ -95,7 +96,7 @@ trait GHPullRequestsSpec[T] extends BaseIntegrationSpec[T] { | |||
| "PullRequests >> ListReviews" should "return a right response when a valid pr is provided" in { | |||
| val response = | |||
| Github(accessToken).pullRequests | |||
| .listReviews(validRepoOwner, validRepoName, validPullRequestNumber) | |||
| .listReviews(validRepoOwner, validRepoName, validPullRequestNumber, None) | |||
| @@ -107,7 +108,7 @@ trait GHPullRequestsSpec[T] extends BaseIntegrationSpec[T] { | |||
| it should "return error when an invalid repo name is passed" in { | |||
| val response = | |||
| Github(accessToken).pullRequests | |||
| .listReviews(validRepoOwner, invalidRepoName, validPullRequestNumber) | |||
| .listReviews(validRepoOwner, invalidRepoName, validPullRequestNumber, None) | |||
| accessToken, | ||
| s"repos/$owner/$repo/pulls/$number/files", | ||
| headers, | ||
| pagination = pagination) |
There was a problem hiding this comment.
was this scalafmt? the indent seems weird
There was a problem hiding this comment.
Yes.
I stopped running Scalafmt because the repo was overall not well formatted; fixed this manually.
|
@BenFradet @fedefernandez I've addressed all comments; please take another look. |
| * - sort: What to sort results by. Can be either `created`, `updated`, `popularity` (comment count) | ||
| * or `long-running` (age, filtering by pulls updated in the last month). Default: `created` | ||
| * - direction: The direction of the sort. Can be either `asc` or `desc`. | ||
| * Default: `desc` when sort is created or sort is not specified, otherwise `asc`. |
There was a problem hiding this comment.
Could you please add the parameter description here?
| * @param headers optional user headers to include in the request | ||
| * @param owner of the repo | ||
| * @param repo name of the repo | ||
| * @param number of the pull request for which we want to list the files |
There was a problem hiding this comment.
Could you please add the parameter description here?
| * @param headers Optional user header to include in the request | ||
| * @param owner Owner of the repo | ||
| * @param repo Name of the repo | ||
| * @param pullRequest ID number of the PR to get reviews for. |
There was a problem hiding this comment.
Could you please add the parameter description here?
|
Thanks @lloydmeta! |
Closes #197
Something funky going on with Scalafmt here...