RFR: 1575: Adjust the pull request APIs in HostedRepository
Erik Joelsson
erikj at openjdk.org
Fri Sep 2 17:22:22 UTC 2022
On Thu, 1 Sep 2022 12:55:55 GMT, Guoxiong Li <gli at openjdk.org> wrote:
> Hi all,
>
> This patch mainly has the following changes in `HostedRepository` and its sub-classes. And the usages of the changed methods are also changed.
>
> 1. Change the name of the `pullRequests(ZonedDateTime updatedAfter)` to `pullRequestsAfter` so that it is consistent as the `openPullRequestsAfter`.
> 2. Add the new api `openPullRequests` to get all the open pull requests.
> 3. Change the implementation of the method `pullRequests()` to get all the pull requests (included open and closed)
>
> Thanks for taking the time to review.
>
> Best Regards,
> -- Guoxiong
When this is reviewed, could you `/integrate defer` it so that I can control when it gets integrated? I will need to prepare a patch for dependent internal source that has to go in at the same time.
forge/src/main/java/org/openjdk/skara/forge/github/GitHubRepository.java line 147:
> 145:
> 146: @Override
> 147: public List<PullRequest> pullRequests() {
I get why you want this method for symmetry, but in reality we could never actually call it in Skara if it would actually return all pull requests for a repo. The potential result set is just too large. The JDK repo has over 10k already. This is why the old `pullRequests(ZonedDateTime)` has the maxPages(1) limit. This method needs that too.
forge/src/main/java/org/openjdk/skara/forge/github/GitHubRepository.java line 158:
> 156: public List<PullRequest> openPullRequests() {
> 157: // The default `state` parameter is `open` in GitHub which is not same as `GitLab`.
> 158: // Here, the `state` parameter is also added to avoid misunderstanding.
I don't think we need this comment. Same for lines 181:182.
forge/src/main/java/org/openjdk/skara/forge/gitlab/GitLabRepository.java line 135:
> 133: @Override
> 134: public List<PullRequest> pullRequests() {
> 135: return request.get("merge_requests")
We need a maxPages limit here too. I see it's missing for pullRequestsAfter for Gitlab as well, which is a bug and should also be fixed.
-------------
PR: https://git.openjdk.org/skara/pull/1367
More information about the skara-dev
mailing list