Moving backend specific behavior from Page to Iterator. by dhermes · Pull Request #2545 · googleapis/google-cloud-python

dhermes · 2016-10-14T21:13:54Z

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.

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.

theacodes

This looks good to me with one minor thing.

Sign in to view

 """Get the next value in the iterator."""
 item = six.next(self._item_iter)
- result = self._item_to_value(item)
+ result = self._parent._item_to_value(item)


theacodes · 2016-10-14T21:41:53Z

SGTM

On Fri, Oct 14, 2016 at 2:40 PM Danny Hermes notifications@github.com
wrote:

@dhermes commented on this pull request.
In core/google/cloud/iterator.py
#2545:
 def next(self):
"""Get the next value in the iterator."""
item = six.next(self._item_iter)
 result = self._item_to_value(item)
 result = self._parent._item_to_value(item)
No actually it's good for it to be called here. Look at my snippet in the
top of iterator.py.
It allows both paradigms to live side by side:
Gimme everything I don't care about paging
I want to page manually
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#2545,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAPUc6dQRe9jnxKILbN391KamqEVFAFYks5qz_bAgaJpZM4KXcnX
.

Sign in to view

+ [
+ <MyItemClass at 0x7fea740abdd0>,
+ <MyItemClass at 0x7fea740abe50>,
+ ]


Sign in to view

+ blob._set_properties(item)
+ return blob
+
+ def _update_page(self):


The previous implementation may catch users off guard since the iterator.page access may also update the value before access. In addition, this PR removed the _update_page() / next_page() subclass behavior in _BlobIterator. Over-riding that method was never intended. Instead makes a non-public class attribute _PAGE_CLASS that can be replaced with Page subclasses. This can be revisited if more implementations require custom behavior on Page creation / Page.__init__.

dhermes · 2016-10-15T00:12:51Z

@jonparrott PTAL

Sign in to view

- if self.has_next_page():
- response = self._get_next_page_response()
- self._page = self.PAGE_CLASS(self, response)
+ if self._page is NO_MORE_PAGES:


theacodes · 2016-10-15T00:35:59Z

I'm pretty okay with the combination of page is None and not has_next_page()

On Fri, Oct 14, 2016, 5:17 PM Danny Hermes notifications@github.com wrote:

@dhermes commented on this pull request.
In core/google/cloud/iterator.py
#2545 (review)
:
 """
 if self.page is not None and self.page.remaining > 0:
 return
 if self.has_next_page():
 response = self._get_next_page_response()
 self._page = self.PAGE_CLASS(self, response)
 if self._page is NO_MORE_PAGES:
@jonparrott https://github.com/jonparrott I don't really want to
introduce this sentinel. I just didn't like that page is None for an
unstarted iterator and page is None for an exhausted iterator. Any good
ideas?
In terms of checking of next_page() can be done, I could do without the
sentinel. If I didn't have the sentinel, then the NO_MORE_PAGES exception
would correspond to self._page is None and not self.has_next_page().
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2545 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAPUc8iSx-mpLvTuk88zwchxKoRC5DDbks5q0BuMgaJpZM4KXcnX
.

theacodes · 2016-10-15T01:35:30Z

Alternatively, why does iterator.next_page() return True for no results?

Also renaming next_page() to update_page() on Iterator and dropping any return value from that method. Also throwing an AttributeError if the page is unset on @Property access.

This is because Iterator.page combined with Iterator.update_page() can provide the same thing and has_next_page() is really an implementation detail. Done via: $ git grep -l has_next_page | > xargs sed -i s/has_next_page/_has_next_page/g

dhermes · 2016-10-16T18:18:38Z

@jonparrott PTAL. I did what we discussed out-of-band yesterday. Instead of using None for both unstarted and no more pages, I used a private sentinel for _UNSET page (will never come via the @property since it raises AttributeError). I also made _has_next_page() private in another commit since Iterator.page combined with Iterator.update_page() cover the same functionality without exposing the has_next_page() helper used in iteration.

daspecster · 2016-10-16T20:38:36Z

@dhermes, why the name change from next_page() to update_page()?

dhermes · 2016-10-17T01:57:08Z

@daspecster Because next_page() sounds like it will return a page, but the method is intended to update the local state

daspecster · 2016-10-17T13:14:43Z

@dhermes yeah I guess I was confused by this example https://github.com/GoogleCloudPlatform/google-cloud-python/pull/2545/files#diff-1e128fc6c096e8635aabf4ba0c484200R90.
Not to complicate this PR but would it make sense to have a next_page() that actually grabs the next page? ITSM to that next_page() would be a really common case.

dhermes · 2016-10-17T17:12:31Z

@daspecster Let's continue design discussion in #2548

Moving backend specific behavior from Page to Iterator.

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 added the api: core label Oct 14, 2016

dhermes assigned daspecster, tseaver and theacodes Oct 14, 2016

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 14, 2016

theacodes approved these changes Oct 14, 2016
View reviewed changes

dhermes commented Oct 14, 2016
View reviewed changes

Comment threadcore/google/cloud/iterator.py
[
<MyItemClass at 0x7fea740abdd0>,
<MyItemClass at 0x7fea740abe50>,
]
This comment was marked as spam.
Sign in to view
This comment was marked as spam.
Sign in to view

dhermes commented Oct 14, 2016
View reviewed changes

Comment threadstorage/google/cloud/storage/bucket.py Outdated
blob._set_properties(item)
return blob

def _update_page(self):
This comment was marked as spam.
Sign in to view

dhermes commented Oct 15, 2016
View reviewed changes

Comment threadcore/google/cloud/iterator.py Outdated
if self.has_next_page():
response = self._get_next_page_response()
self._page = self.PAGE_CLASS(self, response)
if self._page is NO_MORE_PAGES:
This comment was marked as spam.
Sign in to view

dhermes added 2 commits October 16, 2016 11:12

Removing NO_MORE_PAGES sentinel and just making None mean no more pages.
6d0f907
Also renaming next_page() to update_page() on Iterator and dropping any return value from that method. Also throwing an AttributeError if the page is unset on @Property access.

Making Iterator.has_next_page() non-public.
8226323
This is because Iterator.page combined with Iterator.update_page() can provide the same thing and has_next_page() is really an implementation detail. Done via: $ git grep -l has_next_page | > xargs sed -i s/has_next_page/_has_next_page/g

theacodes approved these changes Oct 17, 2016
View reviewed changes

theacodes mentioned this pull request Oct 17, 2016
Figure out the right level of abstraction for iterators. #2548
Closed

dhermes merged commit 0aca3f6 into googleapis:masterOct 17, 2016

dhermes deleted the revamp-iterator-2 branch October 17, 2016 17:12

dhermes mentioned this pull request Nov 14, 2016
Upgrading core to version to 0.21.0. #2733
Merged

dhermes mentioned this pull request Dec 9, 2016
Update versions for mega-release. #2846
Merged

richkadel pushed a commit to richkadel/google-cloud-python that referenced this pull request May 6, 2017
Merge pull requestgoogleapis#2545from dhermes/revamp-iterator-2
cec376a
Moving backend specific behavior from Page to Iterator.

theacodes unassigned daspecster Sep 28, 2018

Uh oh!

Conversation

dhermes commented Oct 14, 2016

Uh oh!

theacodes left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

theacodes commented Oct 14, 2016

@dhermes commented on this pull request.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

dhermes commented Oct 15, 2016

Uh oh!

This comment was marked as spam.

Uh oh!

theacodes commented Oct 15, 2016

@dhermes commented on this pull request.

Uh oh!

theacodes commented Oct 15, 2016

Uh oh!

dhermes commented Oct 16, 2016

Uh oh!

daspecster commented Oct 16, 2016

Uh oh!

dhermes commented Oct 17, 2016

Uh oh!

daspecster commented Oct 17, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dhermes commented Oct 17, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

daspecster commented Oct 17, 2016•
edited
Loading