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

Allow enumerable pagination #1728

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jonabc
Copy link

@jonabc jonabc commented Dec 3, 2024

Resolves #1722

Before the change?

Octokit-provided paginated API requests is only available via the auto paginate functionality, which queries for all pages internally and provides no user control for things like lazy page requests or mapping of page values to reduce memory usage.

After the change?

Note I wanted to feel out whether this approach was worth pursuing before adding full test coverage and commenting all the new logic. I'm happy to do all of that if y'all would consider taking this change.

I've added a new paginate configurable option to the octokit client that, when set, returns an enumerable pager. Either the new paginate value or the existing auto_paginate value can also be specified using custom pagination options provided directly on the client call like client.list_app_installations(..., pagination: { auto_paginate: true })

The pager provides additional custom functionality over a strict enumerator:

  • letting callers provide a max_pages value to limit the number of pages retrieved from the server
  • access the total number of pages for the API request, as specified by the last header link
  • include the raw response object in the enumeration using with_response, similar to enumerable's with_index method

Pull request checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

  • Yes
  • No

I've made the logic non-breaking, and have the full test suite passing in my local environment (hopefully in CI as well 😅 ). TBH though I think having an enumerable makes the auto_paginate option unnecessary. Maybe it should be marked deprecated and removed in the next major version bump?


Copy link

github-actions bot commented Dec 3, 2024

👋 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 Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

[FEAT]: Change auto_paginate to return an enumerator instead of getting all pages
1 participant