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