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

Paginate when retrieving check runs and check suites #1321

Closed
wants to merge 1 commit into from

Conversation

a2ikm
Copy link
Contributor

@a2ikm a2ikm commented Jan 28, 2021

As far as I've investigated, the following check APIs return the link headers:

This PR allows auto_paginate in them.

Copy link
Member

@tarebyte tarebyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution!

We let the client decide whether they want to auto paginate or not in their settings https://github.com/octokit/octokit.rb#pagination. This helps with rate limiting an other factors.

Would you mind just updating the calls to use paginate without the block?

I took a look at The API response, and now I see why you had to change the block because we list checks as an array in the payload body:

https://docs.github.com/en/rest/reference/checks#list-check-runs-in-a-check-suite

This is exactly what we do with

paginate("user/installations", options) do |data, last_response|
data.installations.concat last_response.data.installations
end

Mind just adding tests 😅

@a2ikm
Copy link
Contributor Author

a2ikm commented Jan 29, 2021

Thanks for the comment!
Yes, that's exactly why I added the block 😄

Mind just adding tests 😅

Okay, I'll add tests 👍
It may take a time because we should update VCR cassetts to include two or more documents in the lists.

@kou
Copy link

kou commented Mar 17, 2022

@a2ikm Do you still want to add tests for this changes?

@matiasalbarello @lhmzhou Do you want to help it?

@matiasalbarello
Copy link
Contributor

@tarebyte would it be possible to get this PR reviewed? Is related to this current one, but with added tests. Thanks in advance!

@tarebyte
Copy link
Member

tarebyte commented May 9, 2022

@matiasalbarello I'm actually no longer maintaining Octokit #1365 apologies for the delay.

@a2ikm
Copy link
Contributor Author

a2ikm commented May 18, 2022

@kou
Sorry for my too late reaction.
I have no time to add tests for now.

On the other hand, @matiasalbarello's PR looks good.
So please continue with it 🙏

@nickfloyd
Copy link
Contributor

Closing this one in favor of: @matiasalbarello's PR. Thank you @a2ikm for your effort here and for getting this work in motion. ❤️

@nickfloyd nickfloyd closed this May 25, 2022
@nickfloyd nickfloyd added Status: Stale Used by stalebot to clean house and removed stale labels Oct 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Stale Used by stalebot to clean house
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants