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

Make PagedListResponse iterable #221

Merged
merged 4 commits into from Dec 11, 2018

Conversation

michaelbausor
Copy link
Contributor

PTAL, and let me know whether we need more or clearer documentation. cc @sirtorry, @tmatsuo

Closes #153

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 11, 2018
@codecov-io
Copy link

codecov-io commented Dec 11, 2018

Codecov Report

Merging #221 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #221      +/-   ##
==========================================
+ Coverage   85.88%   85.89%   +0.01%     
==========================================
  Files          68       68              
  Lines        2550     2552       +2     
==========================================
+ Hits         2190     2192       +2     
  Misses        360      360
Impacted Files Coverage Δ
src/PagedListResponse.php 75% <100%> (+3.57%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a69d51e...67e9284. Read the comment docs.

Copy link
Contributor

@bshaffer bshaffer left a comment

Choose a reason for hiding this comment

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

This looks great!

Only thing I can think of is if we want to show the recommended way to NOT iterate over all results, e.g.:

$page = $pagedListResponse->getPage();
if ($page->hasNextPage()) {
    $nextPage = $page->getNextPage();
}

Can you think of a place it would make sense to put this?

@michaelbausor
Copy link
Contributor Author

@bshaffer added examples in the class comments - WDYT?

Copy link
Contributor

@dwsupplee dwsupplee left a comment

Choose a reason for hiding this comment

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

Looks nice, just one minor nit!

* ```
* $pagedListResponse = $client->getList(...);
* foreach ($pagedListResponse as $element) {
* // doSomethingWith($element);

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

Copy link
Contributor

@jdpedrie jdpedrie left a comment

Choose a reason for hiding this comment

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

👏

@michaelbausor michaelbausor merged commit 7aa8efc into googleapis:master Dec 11, 2018
@michaelbausor michaelbausor deleted the paged-iterator branch December 11, 2018 20:59
@bshaffer
Copy link
Contributor

What a guy!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants