Skip to content

Skip and page selection support in PagedIterator/PagedIterable #348

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

Open
JakubKahovec opened this issue Mar 20, 2017 · 11 comments · May be fixed by #1730
Open

Skip and page selection support in PagedIterator/PagedIterable #348

JakubKahovec opened this issue Mar 20, 2017 · 11 comments · May be fixed by #1730

Comments

@JakubKahovec
Copy link

Hello,

when listing users the GitHub Api provides a parameter since which allows you to specify an id of the user you've seen the last, when you start listing again (i.e after a crash) you skip the users you've seen. It'd great to have this parameter in the api i.e github.listUsers(sinceUserId). Do you think it would be feasible to add it there ?

Thank you

Jakub

@kohsuke
Copy link
Collaborator

kohsuke commented Sep 9, 2017

Translating the requirement: it'd be convenient to allow PagedIterator to skip forward without actually retrieving the objects in between.

@kohsuke kohsuke changed the title Using 'since' parameter when listing users Skip support in PagedIterator Sep 9, 2017
@nodoze
Copy link

nodoze commented Oct 5, 2017

I think the PageIterator simply needs to implement the "page" parameter to skip to a particular page. It support size but not page from what I can tell.

https://developer.github.com/v3/#pagination

nodoze pushed a commit to nodoze/github-api that referenced this issue Oct 6, 2017
…er for

the main search. The alternative is to implement it at the page
PagedIterable level which will require changes throughout all the
classes that reference it. Since is primarily a search issue this
solution might suffice.
@bitwiseman
Copy link
Member

bitwiseman commented Oct 11, 2019

@nodoze
Yes, there is enough information for PagedIterable to know how many pages it has, you only have get the first page in order to know the total number of pages. Seems like something that could be added without breaking anything.

@bitwiseman
Copy link
Member

In #448, another scenario was mentioned - restarting a search from a specific page. One thing to keep in mind for searches is that the underlying data may have changed. At that point the page position of items may have changed. This a generally problem for any paged query though, so maybe that is something that we can leave to consumers of the API to figure out for themselves.

@bitwiseman bitwiseman changed the title Skip support in PagedIterator Page selection support in PagedIterator/PagedIterable Nov 24, 2020
@bitwiseman bitwiseman changed the title Page selection support in PagedIterator/PagedIterable Skip and page selection support in PagedIterator/PagedIterable Nov 24, 2020
@bitwiseman
Copy link
Member

bitwiseman commented Nov 24, 2020

I think it would make sense to add a GitHubPaginator class that handles page traversal and navigation. PagedIterable would have a method paginator() that would return the new class and support going to a specific page, next, previous, etc.

@bitwiseman
Copy link
Member

bitwiseman commented Jul 26, 2021

See #1197
Other users would like a method that returns an iterator that only returns results for a specific page range: pageRange(start, end). Results would be returned only within that page range.

I'm thinking the following methods on the iterable:
startAtPage(long)
endAfterPage(long)
pageRange(long, long)
pageIterator()
pageIterator(start_page)
pageIterator(start_page, end_page)

And adding methods to the iterator and page iterator:
hasPrevious()
previous()
getCurrentPageNumber()
getPageCount()
first()
last()

@anujhydrabadi
Copy link
Contributor

@bitwiseman Hey! Thank you for this library, works great! I ended up not using this library for integrating with the paginated GitHub APIs because of the lack of support for specific page number, and wrote my own simple client instead.

I'd like to implement this, perhaps starting with these new methods in PagedIterator, using just the Link response header, then moving to the other features that would use the page param.

hasPrevious() previous() getCurrentPageNumber() getPageCount() first() last()

Any updates to the design you thought of in the previous comments, about the new class GitHubPaginator that is accessible from PagedIterator? Looks fine to me, just wanted to confirm given that it has been more than 2 years since the comments.

Thanks,
Anuj

@bitwiseman
Copy link
Member

Please go ahead.

@anujhydrabadi
Copy link
Contributor

Hey, could I have access to hub4j-test-org organization for testing?

I have implemented two new classes Paginator and GitHubPaginator analogous to PagedIterator and GitHubPageIterator respectively, accessible from PagedIterable. I have kept PagedIterator and GitHubPageIterator as they were.

We can have a discussion when I raise the PR for this, testing in progress.

@bitwiseman
Copy link
Member

@anujhydrabadi
Invitation sent for test org.

@anujhydrabadi anujhydrabadi linked a pull request Oct 19, 2023 that will close this issue
11 tasks
@bitwiseman
Copy link
Member

bitwiseman commented Oct 20, 2023

This is some additional documentation in this area:
https://docs.github.com/en/rest/guides/using-pagination-in-the-rest-api?apiVersion=2022-11-28

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants