1209 GHCompare supports per_page and page params

[ravingbool]

GHCompare supports per page and page+params

Fixes #1209

[codecov]

Codecov Report

Merging #1214 (a60a93f) into main (e317813) will increase coverage by 0.68%.
The diff coverage is 83.33%.

@@             Coverage Diff              @@
##               main    #1214      +/-   ##
============================================
+ Coverage     74.26%   74.95%   +0.68%     
- Complexity     1882     1903      +21     
============================================
  Files           188      188              
  Lines          6202     6232      +30     
  Branches        368      371       +3     
============================================
+ Hits           4606     4671      +65     
+ Misses         1374     1338      -36     
- Partials        222      223       +1     

Impacted Files	Coverage Δ
src/main/java/org/kohsuke/github/GHCompare.java	90.62% <78.57%> (+85.49%)	⬆️
src/main/java/org/kohsuke/github/GHRepository.java	68.25% <100.00%> (+0.86%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e317813...a60a93f. Read the comment docs.

[ravingbool]

@bitwiseman please run the checks again, I pushed the test

[ravingbool]

@bitwiseman thank you for changing the test, please merge the pull request, seems like the checks are ok

[ravingbool]

It is all ok with MR?

[bitwiseman]

@ravingbool
Sorry for the delay, I'm currently on vacation.

This is a harder problem to solve right than it might appear at first. What you have done is okay and exposes this new feature of the endpoint, but it does so in a way that is inconsistent with the rest of the APIs in this library.

All the other endpoints that support per_page and page parameters use PagedIterable to iterate over items. However, those endpoints also for the most part return simple lists. There are also some endpoints that return pages as relatively sparse objects that then contain lists.

For example GHRepository.getCheckRuns returns PagedIterable<GHCheckRun>:

github-api/src/main/java/org/kohsuke/github/GHRepository.java

Lines 2027 to 2033 in 4247c8a

	public PagedIterable<GHCheckRun> getCheckRuns(String ref) throws IOException {
	GitHubRequest request = root.createRequest()
	.withUrlPath(String.format("/repos/%s/%s/commits/%s/check-runs", getOwnerName(), name, ref))
	.withPreview(ANTIOPE)
	.build();
	return new GHCheckRunsIterable(this, request);
	}

This actually an instance of GHCheckRunsIterable which pages through GHCheckRunsPage objects.

I think the right way to address this will be to add GHCompare.listCommits() method that returns a PagedIterable<Commit> and internally implements something similar GHCheckRunsIterable that uses GHCompare objects as the pages.

Then change your new method to be GHRepository.getCompare(String id1, String id2, int default_commits_per_page). The query for that would set per_page to 1 and set a new default_commits_per_page field on the GHCompare instance. This would result in only one commit being returned as part of the that query, minimizing extra data transmission.
If default_commits_per_page field is set, GHCompare.listCommits() would have it's per_page set to the that by default and GHCompare.getCommits() would call listCommits().getArray() instead of it's current behavior.

You'll also need a few new tests for this behavior. Specifically, at least one test that has two pages of results.

Does this make sense? Do you have time to do this additional work?

[ravingbool]

@bitwiseman well, I didn't suppose that it would so a lot of work. Can we merge it as is now and make a follow up for it? Because I would like to have a new version with this functionality as soon as possible. I will refactor it in the future, but now I would like to have it as is, pleeeease @bitwiseman

[bitwiseman]

@ravingbool
Sorry, adding methods that we intend to remove "as soon as possible" is counter to policy for this library. However, I recognize that you've worked on this in good faith and need the functionality badly. I've pushed a commit that does basically what I described with minor changes. I've modified the test to show how the modified functionality works. What do you think?

[ravingbool]

@bitwiseman
Thank you very much, it is great!