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.

Sign up for GitHub

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

Jump to bottom

Re-architect Iterator class. #2531

Merged
merged 13 commits into from
Oct 14, 2016
Merged

Re-architect Iterator class. #2531

merged 13 commits into from
Oct 14, 2016

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Oct 12, 2016

This PR should definitely reviewed commit-by-commit.

Primary changes are:

  • Have an iterable Page base-class for iterating over a single page of a result set
  • Move most custom iterator logic back up into the base page and iterator classes

@dhermes dhermes added api: storage Issues related to the Cloud Storage API. api: core api: cloudresourcemanager Issues related to the Resource Manager API. labels Oct 12, 2016
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 12, 2016
dhermes added 13 commits October 12, 2016 01:05
@dhermes
Implementing basic Page class to help with Iterator.
6636714 
Intended to hold and slice up state that has already been
retrieved from the server.
@dhermes
Collapsing get_items_from_response behavior into Page subclasses.
e9d600a
@dhermes
Collapsing Page __init__ functionality into base class.
819e644
@dhermes
Replacing get_items_from_response() with a page class.
afc0662
@dhermes
Making path an optional argument to Iterator.
ff85b24
@dhermes
Swapping iterator's _next_item() for _item_to_value().
5326c88 
This is so that the owned items iterator management could
be done on the base class and the child classes just
need to worry about converting the JSON values to whatever
type is required.
@dhermes
Exposing the current page in iterator.
f4c2bcd
@dhermes
Removing RuntimeError from iterator's get_next_page_response().
4988a2a 
This state can never happen since a StopIteration will occur
before the method would ever be called without a token.
@dhermes
Adding a page reset to iterator's reset.
736c3a0 
Also doing a tiny re-org in constructor to separate the
attributes which change and those which don't.
@dhermes
Updating storage system test after Iterator refactor.
122ec89
@dhermes
Lint cleanup: unused imports.
c6218a2
@dhermes
Removing unused MethodIterator.
c92961d
@dhermes
Implementation hiding in Iterator class.
d9824c6 
Making JSON API call helpers non-public. This reduces the
interface to methods relevant to iterating.
@dhermes dhermes mentioned this pull request Oct 12, 2016
Closed
theacodes
theacodes reviewed Oct 14, 2016
View reviewed changes
Copy link
Contributor

@theacodes theacodes left a comment •
edited
Loading

Choose a reason for hiding this comment

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

My one concern is how a service like datastore will work.

This abstraction seems to completely hides cursors, but cursors often need to be passed between client and server in datastore (think client-side pagination). I'm wondering how that would fit in here?

core/google/cloud/iterator.py
"""The :class:`Page` is an iterator."""
return self

def _item_to_value(self, item):

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

core/google/cloud/iterator.py
return self._num_items

@property
def remaining(self):

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@dhermes
Copy link
Contributor Author

dhermes commented Oct 14, 2016

My one concern is how a service like datastore will work.

Datastore has it's own specialized Iterator. For the time being, the google.cloud.iterator.Iterator class is for JSON-based APIs that define a nextPageToken in response pages.

Though a "next step" would be to make the iterator support gRPC / GAPIC generated classes, so we may be able to tackle datastore there. (Just not in this PR.)

@daspecster
Copy link
Contributor

Sorry I haven't responded on this @dhermes. I don't have much feedback though.

You actually just answered the question I did have.

Though a "next step" would be to make the iterator support gRPC / GAPIC generated classes, so we may be able to tackle datastore there. (Just not in this PR.)

@theacodes
Copy link
Contributor

LGTM

@dhermes dhermes merged commit 9a5ddd5 into googleapis:master Oct 14, 2016
@dhermes dhermes deleted the revamp-iterator branch October 14, 2016 19:09
dhermes added a commit to dhermes/google-cloud-python that referenced this pull request Oct 14, 2016
@dhermes
Moving backend specific behavior from Page to Iterator.
49532f4 
This is to lower the burden on implementers. The previous
approach (requiring a Page and Iterator subclass) ended
up causing lots of copy-pasta docstrings that were just
a distraction.

Follow up to googleapis#2531.
@dhermes dhermes mentioned this pull request Oct 14, 2016
Merged
@dhermes dhermes mentioned this pull request Nov 14, 2016
Merged
@dhermes dhermes mentioned this pull request Dec 9, 2016
Merged
@tseaver tseaver mentioned this pull request Feb 6, 2017
Closed
richkadel pushed a commit to richkadel/google-cloud-python that referenced this pull request May 6, 2017
@dhermes
Merge pull request googleapis#2531 from dhermes/revamp-iterator
ba13299 
Re-architect Iterator class.
richkadel pushed a commit to richkadel/google-cloud-python that referenced this pull request May 6, 2017
@dhermes
Moving backend specific behavior from Page to Iterator.
9c1a083 
This is to lower the burden on implementers. The previous
approach (requiring a Page and Iterator subclass) ended
up causing lots of copy-pasta docstrings that were just
a distraction.

Follow up to googleapis#2531.
parthea pushed a commit that referenced this pull request Jun 4, 2023
@dhermes
Moving backend specific behavior from Page to Iterator.
ab05218 
This is to lower the burden on implementers. The previous
approach (requiring a Page and Iterator subclass) ended
up causing lots of copy-pasta docstrings that were just
a distraction.

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

@theacodes theacodes theacodes left review comments

Assignees

@tseaver tseaver

Labels
api: cloudresourcemanager Issues related to the Resource Manager API. api: core api: storage Issues related to the Cloud Storage API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dhermes @daspecster @theacodes @tseaver @googlebot