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

Update iterator to take a method and **kwargs #1554

Closed
tseaver opened this issue Mar 1, 2016 · 6 comments
Closed

Update iterator to take a method and **kwargs #1554

tseaver opened this issue Mar 1, 2016 · 6 comments
Assignees
Labels
api: bigquery Issues related to the BigQuery API. api: cloudresourcemanager Issues related to the Resource Manager API. api: core api: dns Issues related to the Cloud DNS API. api: pubsub Issues related to the Pub/Sub API. api: storage Issues related to the Cloud Storage API.

Comments

@tseaver
Copy link
Contributor

tseaver commented Mar 1, 2016

gcloud.iterator.Iterator is a base class, relying on a path for the API method. I think it would be clearer if it wrapped around a method of a client (or other domain object), where that method takes page_token, page_size, etc., along with other named arguments in **kw. We could then ditch all the derived classes, and just use the one, common implementation. E.g.:

from gcloud.iterator import Iterator
from gcloud.pubsub import Client

client = Client()
topics = list(Iterator(client.list_topics))
subs = list(Iterator(client.list_subscriptions, topic_name='my-topic'))
@tseaver tseaver added api: storage Issues related to the Cloud Storage API. api: bigquery Issues related to the BigQuery API. api: dns Issues related to the Cloud DNS API. api: pubsub Issues related to the Pub/Sub API. api: core api: cloudresourcemanager Issues related to the Resource Manager API. search labels Mar 1, 2016
@dhermes
Copy link
Contributor

dhermes commented Mar 2, 2016

SGTM in spirit. Iterator was something we inherited, so it never really had an encompassing design discussion.

Also 👍 to making it no longer public.

@tseaver
Copy link
Contributor Author

tseaver commented Mar 3, 2016

It needs to be public: we expect client code to instantiate the Iterator class directly. We would likely be removing the various subclasses, however.

@dhermes
Copy link
Contributor

dhermes commented Mar 3, 2016

Ahh I didn't realize we'd use the class directly. Works for me.

@rimey
Copy link
Contributor

rimey commented Apr 9, 2016

There are really two points here:

  1. Client code could request "infiniscrolling" by wrapping a paging-aware list_* bound method in a MethodIterator. This proposal and others are being discussed in Allow easy paging for list operations #895.
  2. Having to subclass Iterator is burdensome and unnecessary.

The second point is worthy of attention independently of the first. For example, the current list_projects method could be implemented more concisely in terms of MethodIterator.

For infiniscrolling, a simple generator would do the job as well as a class:

def chain_pages(function, **kwargs):
    token = None
    while True:
        items, token = function(page_token=token, **kwargs)
        for item in items:
            yield item
        if token is None:
            break

@dhermes
Copy link
Contributor

dhermes commented Apr 11, 2016

FWIW: We've got a pending issue (#1467) which points out that a bit more logic is needed when max_results is used.

@tseaver
Copy link
Contributor Author

tseaver commented Feb 6, 2017

Obsoleted by #2531 and subsequent PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquery Issues related to the BigQuery API. api: cloudresourcemanager Issues related to the Resource Manager API. api: core api: dns Issues related to the Cloud DNS API. api: pubsub Issues related to the Pub/Sub API. api: storage Issues related to the Cloud Storage API.
Projects
None yet
Development

No branches or pull requests

3 participants