Skip to content
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

"GET /repos/{owner}/{repo}/commits/{ref}" can paginate the "files" property #155

Open
gr2m opened this issue Sep 3, 2020 · 6 comments
Open
Labels
Good first issue Good for contributors new to Octokit Status: Up for grabs Issues that are ready to be worked on by anyone

Comments

@gr2m
Copy link
Contributor

gr2m commented Sep 3, 2020

See https://docs.github.com/en/rest/reference/repos#get-a-commit

If there are more than 300 files in the commit diff, the response will include pagination link headers for the remaining files, up to a limit of 3000 files. Each page contains the static commit information, and the only changes are to the file listing.

It's an odd case. Only the "files" key array will be paginated, but unlike the other endpoints that paginate that return an object instead of an array, this one will include lots of other keys. I'm not sure if that case is covered by https://github.com/octokit/plugin-paginate-rest.js/blob/92ed1aab99fe8045fe2c23b21308390471d0c8f5/src/normalize-paginated-list-response.ts

If someone would like to give this a go, I'd be happy to advice. Comment below if you have any questions. The first step would be to add a test to https://github.com/octokit/plugin-paginate-rest.js/blob/master/test/paginate.test.ts, then start a pull request. We can continue the discussion there

@gr2m gr2m added Status: Up for grabs Issues that are ready to be worked on by anyone Good first issue Good for contributors new to Octokit labels Sep 3, 2020
@eladchen
Copy link

Just read that paragraph myself, and it was evident that pagination ought to be broken given that explanation.

I guess I could try and help, how should we proceed?

@gr2m
Copy link
Contributor Author

gr2m commented Sep 12, 2020

Hi Elad,

first step would be to create a failing test for trying to paginate GET /repos/{owner}/{repo}/commits/{ref}, as mentioned above

If someone would like to give this a go, I'd be happy to advice. Comment below if you have any questions. The first step would be to add a test to https://github.com/octokit/plugin-paginate-rest.js/blob/master/test/paginate.test.ts, then start a pull request. We can continue the discussion there

Would you like to give that a try?

@eladchen
Copy link

eladchen commented Sep 12, 2020 via email

@gr2m
Copy link
Contributor Author

gr2m commented Sep 12, 2020

Good question, I don't know. I think we will have to create a test case to see for ourselves. I assume there will be no next header once we reach 300

@pmauri01
Copy link

pmauri01 commented Aug 4, 2022

Was this problem every resolved, we have fond the same issue

@gr2m
Copy link
Contributor Author

gr2m commented Aug 4, 2022

Probably not? It would be a good starting point would be to create or find a commit that changed 300+ files. If it already paginates successfully then we can close this issue. Otherwise we need to record the response headers and create a failing test that replicates the current behavior. Once we have that, we can discuss how to account for this special case

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good first issue Good for contributors new to Octokit Status: Up for grabs Issues that are ready to be worked on by anyone
Projects
None yet
Development

No branches or pull requests

3 participants