feat: Paginate compareCommits and compareCommitsWithBasehead #678
Conversation
|
👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with |
| if (!url && "total_commits" in normalizedResponse.data) { | ||
| const parsedUrl = new URL(normalizedResponse.url); | ||
| const params = parsedUrl.searchParams; | ||
| const page = parseInt(params.get('page') || '1', 10) | ||
| const per_page = parseInt(params.get('per_page') || '250', 10); | ||
| if (page * per_page < normalizedResponse.data.total_commits) { | ||
| params.set('page', String(page + 1)); | ||
| url = parsedUrl.toString(); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Can you explain why this is necessary?
Don't the existing methods do this already?
There was a problem hiding this comment.
As far as I can tell, the other methods rely on response.headers.link, which is not present in responses to compareCommits or compareCommitsWithBasehead
There was a problem hiding this comment.
That seems like an oversight on GitHub's part, their page on pagination says to use the link header
https://docs.github.com/en/rest/using-the-rest-api/using-pagination-in-the-rest-api?apiVersion=2022-11-28#using-link-headers
There was a problem hiding this comment.
Could you add a test that covers this?
Resolves #647
Before the change?
compareCommitsandcompareCommitsWithBaseheadare not correctly paginated.After the change?
compareCommitsandcompareCommitsWithBaseheadare paginatedPull request checklist
Does this introduce a breaking change?
No breaking changes (unless someone is using paginate with
compareCommits, which doesn't work).Please see our docs on breaking changes to help!