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

Add item-per-item iteration so that users don't need to manage "pages" of results #86

Merged
merged 6 commits into from Feb 23, 2018

Conversation

Projects
None yet
2 participants
@HaraldJoerg
Copy link
Contributor

HaraldJoerg commented Feb 12, 2018

This pull request is motivated by the CPAN Pull Request Challenge, http://cpan-prc.org/.
It is a suggestion for an iteration over individual items instead of "pages". Using this iteration, the caller does not need to manage API calls to fetch more pages of results.
The first three commits are small fixes to the docs and tests, which are actually unrelated to pagination. The fourth contains the basic iteration mechanism plus the implementation for the submodule Net::GitHub::V3::Issues, and documentation for this submodule, as well as a first test script.
The other submodules are pretty straightforward and will follow, but I've a different assignment for the rest of the week, and maybe you can review the proposal from what I've done with Issues.pm.

Unfortunately, I am not a regular user of this module, so I'd welcome pointers to typical workflows or relevant reverse dependencies which I could add as tests.

@fayland

This comment has been minimized.

Copy link
Owner

fayland commented Feb 13, 2018

Hello. thanks for the patching.

overall it looks very good even I didn't test it yet.

just wondering why we need close_*?

thanks

@HaraldJoerg

This comment has been minimized.

Copy link
Contributor

HaraldJoerg commented Feb 13, 2018

@fayland

This comment has been minimized.

Copy link
Owner

fayland commented on lib/Net/GitHub/V3/Events.pm in cc209cc Feb 22, 2018

missing $event?

@fayland

This comment has been minimized.

Copy link
Owner

fayland commented Feb 22, 2018

hi, thanks for the PR and it looks good. I'll merge it and release after you fixed the minor docs issue.

@HaraldJoerg

This comment has been minimized.

Copy link
Contributor

HaraldJoerg commented Feb 22, 2018

A section whether, when and why close_xxx methods may be needed has been included in lib/Net/GitHub/V3.pm, next to the existing section about pagination.

What is still open is pagination of search results (Search.pm), which has easy and tricky parts. It is easy to derive the next_ and close_methods from what is there. The JSON result comes as a hashref instead of an arrayref, so we could iterate over the list elements of the 'item' key with a few extra lines. However, the hash response is there for a reason: In addition to the items, it contains information about the total number of hits, and whether there was a timeout on the server side. I consider this information vital: Iterating over millions of hits is a very bad idea, and interpreting incomplete results is a very bad idea as well. In both cases, the caller needs to adjust his query.

So, a possible approach would be to die if the server reports incomplete results, and also to die if the total number of hits exceed a defined threshold (but which threshold is good enough? Should be configurable on creation of the Search object...).

A second approach would store the two values after the first next_repository (or next_issue, or...) in the Search object. The user would need to call next_ once before he can evaluate the values and maybe abort the iteration.

As a third approach, we could add total_xxx and is_complete_xxx methods to each of issues, repositories, code and users.

And, of course, we could leave searching as it is. The page-per-page iteration works just fine, and the two values are returned in the page response so that the caller can (must!) check the.

Do you have any preferences? Maybe we should followup in another pull request?

@fayland

This comment has been minimized.

Copy link
Owner

fayland commented Feb 23, 2018

yes. it's good enough for now. we can leave search first. thanks

@fayland fayland merged commit b494a2b into fayland:master Feb 23, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@fayland

This comment has been minimized.

Copy link
Owner

fayland commented Feb 23, 2018

I just shipped 0.94 to CPAN. thanks for your work!

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