Skip to content

Add a ResultPager object to support pagination of all Api's #61

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

Merged
merged 21 commits into from
Jul 30, 2013
Merged

Add a ResultPager object to support pagination of all Api's #61

merged 21 commits into from
Jul 30, 2013

Conversation

ramondelafuente
Copy link
Contributor

This should fix #55. (close #49, close #53)

The docs have been updated to include a Pagination Support chapter that shows basic usage.

ramondelafuente and others added 18 commits May 27, 2013 15:37
The get method always prepends the baseurl before the requests path.
A check was added to allow for full URLS (starting with baseurl), for following
pagination links.
The api client supports the per_page github parameter for api calls.
Default is NULL and lets github decide the default (currently 30).

$client->setPerPage()
This is the first pass at adding a paginator class for github API requests.
It only works with fetchAll() at the moment, to demonstrate the usage.

Currently:
$paginator = new \Github\ResultPaginator( \Github\Client [client] );
$paginator->fetchAll( [api], [method], [parameters] );

Todo's include:
Paginator is not complete for all possible headers Next, Last, Previous, First
apiClient should be able to return a paginator (i.e. $client->getPaginator() )
CacheHttpClient does not work (does not return Link headers)
The get method always prepends the baseurl before the requests path.
A check was added to allow for full URLS (starting with baseurl), for following
pagination links.
The api client supports the per_page github parameter for api calls.
Default is NULL and lets github decide the default (currently 30).

$client->setPerPage()
This is the first pass at adding a paginator class for github API requests.
It only works with fetchAll() at the moment, to demonstrate the usage.

Currently:
$paginator = new \Github\ResultPaginator( \Github\Client [client] );
$paginator->fetchAll( [api], [method], [parameters] );

Todo's include:
Paginator is not complete for all possible headers Next, Last, Previous, First
apiClient should be able to return a paginator (i.e. $client->getPaginator() )
CacheHttpClient does not work (does not return Link headers)
path is empty in some test cases but strpos can't handle an empty needle so added check if path is not empty
$organizationApi = $client->api('organization');

$paginator = new Github\ResultPager( $client );
$result = $paginator->fetchAll( $organizationApi, 'repositories', 'github );
Copy link
Contributor

Choose a reason for hiding this comment

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

missing closing quote

@stof
Copy link
Contributor

stof commented Jul 11, 2013

you should update the coding standards to follow PSR-2


Check for pervious page:
```php
$paginator->getPrevious();
Copy link
Contributor

Choose a reason for hiding this comment

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

hasPrevious ?

@ramondelafuente
Copy link
Contributor Author

@stof @docteurklein Thanks very much for the feedback!

We'll be fixing all mentioned items in the morning (sorry for the obvious issues) except the ones responded on directly, which could be improved upon by discussion maybe...

@@ -163,7 +163,9 @@ public function put($path, array $parameters = array(), array $headers = array()
*/
public function request($path, array $parameters = array(), $httpMethod = 'GET', array $headers = array())
{
$path = trim($this->options['base_url'].$path, '/');
if ( !empty($this->options['base_url']) AND 0 !== strpos( $path, $this->options['base_url'] )) {
Copy link
Contributor

Choose a reason for hiding this comment

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

&&

Updated coding standard to PSR-2
fixed CS issues and typos
updated docs
changed fetch() and fetchAll() signature to an array of parameters for flexibility in the future
@verschoof
Copy link
Contributor

@cursedcoder @pilot wake up guys, accept some pull requests or deny them.. don't let them catch dust!
I would like to use this in my projects.. (now using a fork..(of a fork)). But some more people would like it I guess..

Doesn't KNPLabs have more collaborators?

@docteurklein
Copy link
Contributor

sure you took all comments in consideration ? then, ready to merge.

@pilot
Copy link
Contributor

pilot commented Jul 30, 2013

@verschoof apply small fix's and it will be merge

Added spaces after comma in an array
@verschoof
Copy link
Contributor

@pilot Alright! Good to merge!

docteurklein added a commit that referenced this pull request Jul 30, 2013
Add a ResultPager object to support pagination of all Api's
@docteurklein docteurklein merged commit 86d58f6 into KnpLabs:master Jul 30, 2013
@ramondelafuente ramondelafuente deleted the resultpager branch July 30, 2013 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants