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

First Class Support for Pagination #259

Closed
shiftkey opened this issue Dec 11, 2013 · 8 comments
Closed

First Class Support for Pagination #259

shiftkey opened this issue Dec 11, 2013 · 8 comments
Labels
Type: Feature New feature or request

Comments

@shiftkey
Copy link
Member

So we have the ability to specify the page and per_page parameters in the GitHub API, like this:

$ curl https://api.github.com/user/repos?page=2&per_page=100

But we don't expose this in Octokit.net

I'd like to take a stab at introducing this for the RepositoriesClient and ObservableRepositoriesClient as this is likely to be something that other clients are going to be interested in.

I've been pondering on the API, and given we use the action GetAll to signify "get all repositories" I think specifying overloads (not optional parameters due to various reasons) is a better approach to this, for those who want to opt-in to the behaviour.

cc @niik @haacked

@niik
Copy link
Contributor

niik commented Dec 11, 2013

👍 ❤️

@haacked
Copy link
Contributor

haacked commented Dec 11, 2013

Is this being driven by a real application need or a theoretical need?

I'm a 👎 on it as far as ObservableRepositoriesClient is concerned. You should simply do:

client.GetReposOrWhateverItIsCalled().Take(100); // This will request enough "pages" to get the 100

As much as possible, we've tried to drive implementation based on actual application needs. We also want to make the API fit the idioms of the language. So the Observable API should match the Rx + Linq idioms and the user shouldn't have to think about paging. Paging should be handled transparently as an implementation detail of the API.

For the Task based APIs, that might be much more difficult and we might need to resort to parameter passing. But it's a lower level API anyways.

Just to be clear, I'm not totally opposed to it, we've just been successful with extracting the implementation patterns based on what we needed in GHfW and I want to continue with that focus. Having a real application need really helps clarify our thinking about how it should be implemented.

/cc @half-ogre

@haacked
Copy link
Contributor

haacked commented Dec 11, 2013

p.s.

For the observable clients, Skip will still issue all the requests. That might be a case where sending the page parameter is more efficient.

We've been using a HATEOAS approach so far which makes it very flexible for clients. The response has the next page. I wonder if we could continue this approach by issuing HEAD requests for Skip.

@niik
Copy link
Contributor

niik commented Dec 11, 2013

Is this being driven by a real application need or a theoretical need?

Yes, I'm the one who got the ball running on this one. I need it for GHfW and @shiftkey was kind enough to help out.

You should simply do client.GetReposOrWhateverItIsCalled().Take(100);

But I still need all the results, I just want them delivered as they come in, I don't want to wait for all requests to be made. Also, looking at the repositories client the methods are just names "GetAll*" and that returns a full collection object once everything is done. I don't see how a .Take would help me here but then again this is the first time I've looked at Octokit so I might be looking at the wrong place entirely.

Paging should be handled transparently as an implementation detail of the API.

As I could actually get the results as they come in as opposed to sitting around waiting for all requests to be made and had a way of cancelling when I got what I need I guess that'd be fine. What if I only want the first page of repositories returned from a search for example though (admittedly contrived example)?

@haacked
Copy link
Contributor

haacked commented Dec 11, 2013

But I still need all the results, I just want them delivered as they come in...

That's how it works today.

client.GetAll().Subscribe(repo => /* Get them as they arrive. Does not wait for them all to be made. */);

The Get* methods return an IObservable<Repository>. https://github.com/octokit/octokit.net/blob/master/Octokit.Reactive/Clients/IObservableRepositoriesClient.cs#L63

@haacked
Copy link
Contributor

haacked commented Dec 11, 2013

Make sure you're using the ObservableGitHubClient.

@niik
Copy link
Contributor

niik commented Dec 11, 2013

That's how it works today. The Get* methods return an IObservable.

Looks like I got confused by our internal wrapper client in GHfW, that was the thing which collected all results. Well, in that case I'm golden. All I have to do is use a Window so that I only update the UI once for each response and not once per repo. Thanks @haacked

@M-Zuber
Copy link
Contributor

M-Zuber commented Aug 24, 2016

Completed with this release

@nickfloyd nickfloyd added Type: Feature New feature or request and removed category: feature labels Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants