-
Notifications
You must be signed in to change notification settings - Fork 97
Add RepositoryClient.listPullRequestsForCommit #100
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.
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #100 +/- ##
============================================
+ Coverage 70.62% 70.76% +0.13%
- Complexity 225 226 +1
============================================
Files 36 36
Lines 851 855 +4
Branches 35 35
============================================
+ Hits 601 605 +4
Misses 225 225
Partials 25 25
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Since this API responds with first 30 prs, should we handle this with pagination?
Like
github-java-client/src/main/java/com/spotify/github/v3/clients/RepositoryClient.java
Lines 258 to 263 in e19601c
public Iterator<AsyncPage<Status>> listCommitStatuses(final String sha, final int itemsPerPage) { | |
// FIXME Use itemsPerPage property | |
final String path = String.format(STATUS_URI_TEMPLATE, owner, repo, sha); | |
log.debug("Fetching commits from " + path); | |
return new GithubPageIterator<>(new GithubPage<>(github, path, LIST_STATUS_TYPE_REFERENCE)); | |
} |
note: But we do not handle pagination for most of the other APIs as well.
Yes I think we don't need this and don't do pagination anywhere else. Let's keep it simple for now and add it if we need it later. I don't think it's ever needed in this case: will there ever be more than 30 PRs containing the same commit? Probably not 😉 |
import java.util.Optional; | ||
import java.util.concurrent.CompletableFuture; | ||
import java.util.concurrent.CompletionException; | ||
import javax.ws.rs.core.HttpHeaders; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an alternative class which we could use com.google.common.net.HttpHeaders
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just copied this from other classes in this project, doesn't matter much I think 🤷
final String path = String.format(COMMIT_PULL_REQUESTS_SHA_URI_TEMPLATE, owner, repo, sha); | ||
|
||
// As of GHE 3.2, this feature is still in preview, so we need to add the extra header. | ||
// https://developer.github.com/changes/2019-04-11-pulls-branches-for-commit/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great addition.
json.fromJson("[" + getFixture("../prs/pull_request_item.json") + "]", LIST_PR_TYPE_REFERENCE)); | ||
when(github.request(eq("/repos/someowner/somerepo/commits/thesha/pulls"), eq(LIST_PR_TYPE_REFERENCE), any())) | ||
.thenReturn(fixture); | ||
final List<PullRequestItem> commits = repoClient.listPullRequestsForCommit("thesha").get(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it return commits? Or pull requests? if the latter is true, should be renamed to pullRequests
? (as list)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
See https://docs.github.com/en/enterprise-server@3.2/rest/commits/commits#list-pull-requests-associated-with-a-commit