plugin-paginate-rest.js icon indicating copy to clipboard operation
plugin-paginate-rest.js copied to clipboard

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

Open gr2m opened this issue 5 years ago • 6 comments

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 avatar Sep 03 '20 20:09 gr2m

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?

eladchen avatar Sep 12 '20 11:09 eladchen

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?

gr2m avatar Sep 12 '20 18:09 gr2m

I can do that. Any idea what the "next" header value is when the files exceed 300?

On Sat, Sep 12, 2020, 21:52 Gregor Martynus [email protected] wrote:

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?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/octokit/plugin-paginate-rest.js/issues/155#issuecomment-691530581, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA3V64L5DKWXF7F6AETDCQLSFO7PRANCNFSM4QV45A7A .

eladchen avatar Sep 12 '20 19:09 eladchen

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

gr2m avatar Sep 12 '20 19:09 gr2m

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

pmauri01 avatar Aug 04 '22 14:08 pmauri01

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

gr2m avatar Aug 04 '22 18:08 gr2m