Skip to content

Making max_results part of the base Iterator class. #2490

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
Merged
dhermes merged 1 commit into from
Oct 4, 2016
Merged

Making max_results part of the base Iterator class. #2490

dhermes merged 1 commit into from
Oct 4, 2016

Conversation

@dhermes
Copy link
Contributor

@dhermes dhermes commented Oct 4, 2016

In the process, also making sure to lower maxResults on subsequent requests.

Fixes #1467.

@dhermes dhermes added api: storage Issues related to the Cloud Storage API. api: core labels Oct 4, 2016
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 4, 2016
dhermes
dhermes commented Oct 4, 2016
View reviewed changes
core/google/cloud/iterator.py Outdated
except StopIteration:
item = self._update_items()
self.num_results += 1
return item

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

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 4, 2016

I'm very open to suggestions on other unit / system tests I could add to this PR. Just wanted to get it out there for discussion's sake.

@dhermes
Making max_results part of the base Iterator class.
78b09c5 
In the process, also making sure to lower maxResults
on subsequent requests.

Fixes googleapis#1467.
@tseaver
Copy link
Contributor

tseaver commented Oct 4, 2016

I would've thought changing to use google.cloud.iterator.MethodIterator everywhere would have been more straightforward (and giving us a clearer path toward a general solution for #895). The derived iterator classes feel like roach motels to me.

@dhermes
Copy link
Contributor Author

dhermes commented Oct 4, 2016

@tseaver This is just a band-aid for #1467. I will probably start from scratch / build on top of MethodIterator when finishing #895.

@tseaver tseaver mentioned this pull request Oct 4, 2016
Closed
@dhermes dhermes merged commit 8c50e41 into googleapis:master Oct 4, 2016
@dhermes dhermes deleted the fix-1467 branch October 4, 2016 21:55
@dhermes dhermes mentioned this pull request Nov 14, 2016
Merged
richkadel pushed a commit to richkadel/google-cloud-python that referenced this pull request May 6, 2017
@dhermes
Merge pull request googleapis#2490 from dhermes/fix-1467
4a82b09 
Making max_results part of the base Iterator class.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@daspecster daspecster daspecster left review comments

@tseaver tseaver tseaver approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@tseaver tseaver

Labels

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.

4 participants

@dhermes @tseaver @daspecster @googlebot