Skip to content

Migrate datastore iterator to use base iterator class - #2706

Merged
dhermes merged 13 commits into
googleapis:masterfrom
dhermes:migrate-datastore-iterator
Nov 9, 2016
Merged

Migrate datastore iterator to use base iterator class#2706
dhermes merged 13 commits into
googleapis:masterfrom
dhermes:migrate-datastore-iterator

Conversation

@dhermes

@dhermesdhermes commented Nov 8, 2016

Copy link
Copy Markdown
Contributor

I considered

  • Making offset and end_cursor read-only public properties on the subclass
  • Making a @property alias start_cursor that just returns the value of next_page_token
  • Making a @property alias limit that just returns the value of max_results

I elected not to make anything new public since the original implementation didn't.

@jonparrott PTAL as well (do you think this will be too jarring a change?).

@dhermesdhermes added api: datastore Issues related to the Datastore API. api: core labels Nov 8, 2016
@googlebotgooglebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 8, 2016
theacodes
theacodes reviewed Nov 8, 2016
View reviewed changes
Comment threaddatastore/google/cloud/datastore/client.py

Using the query iterator's
:meth:`~google.cloud.datastore.query.Iterator.next_page` method:
Using the query iterator

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@dhermes
dhermesforce-pushed the migrate-datastore-iterator branch 2 times, most recently from 22578a8 to 0bc7682CompareNovember 8, 2016 18:02
@theacodes

theacodes commented Nov 8, 2016

Copy link
Copy Markdown
Contributor

@dhermes thanks, the same is clear and doesn't make a common use case too difficult. I'm cool with this.

@dhermes
Factoring protobuf construction out of datastore iterator.
d0abce8
@dhermes
Factoring query processing out of datastore iterator.
01e333f
@dhermes
Moving datastore iterator enum helpers into module scope.
a4af15a
@dhermes
Adding alternate datastore iterator class that inherits from base Ite…
79dca14
…rator.
@dhermes
Adding helper for datastore iterator to convert entities.
f722637
@dhermes
Adding _build_protobuf() helper to datastore alt. iterator.
0f1ee53
@dhermes
Adding _process_query_results helper to alt. datastore iterator.
96fc53b
@dhermes
Implement _next_page() on alternate datastore iterator.
f2b828e
@dhermes
Removing original Iterator class in datastore.
a3f68a4
@dhermes
Renaming datastore AltIterator-->Iterator.
845efc9
@dhermes
Updating datastore system tests to use new iterator.
eec6e9f
@dhermes
Pylint fixes for datastore.
aaaa100
Removing an unused import and also adding a class-level
attribute for ``next_page_token`` which gets set outside of the
constructor (Pylint seems to miss the fact that it gets set
in the constructor of the base class).
@dhermes
Adding doc snippet for paging with cursors.
7a97e00
@dhermes
dhermesforce-pushed the migrate-datastore-iterator branch from 0bc7682 to 7a97e00CompareNovember 8, 2016 20:39
@dhermes

dhermes commented Nov 8, 2016

Copy link
Copy Markdown
ContributorAuthor

@daspecster@tseaver PTAL

daspecster
daspecster reviewed Nov 8, 2016
View reviewed changes
Comment threaddatastore/google/cloud/datastore/client.py
>>> query_iter.next_page_token is None
True

Under the hood this is doing:

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

daspecster
daspecster approved these changes Nov 8, 2016
View reviewed changes

@daspecsterdaspecster left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This LGTM to me as well.

@dhermes

dhermes commented Nov 9, 2016•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Going to merge (waiting waited 24h for other feedback)

@dhermes
dhermes merged commit 1abfcbe into googleapis:masterNov 9, 2016
@dhermes
dhermes deleted the migrate-datastore-iterator branch November 9, 2016 17:08
@dhermesdhermes mentioned this pull request Nov 14, 2016
Merged
@remcohaszingremcohaszing mentioned this pull request Nov 17, 2016
Closed
richkadel pushed a commit to richkadel/google-cloud-python that referenced this pull request May 6, 2017
@dhermes
Merge pull requestgoogleapis#2706from dhermes/migrate-datastore-ite…
5001fac
…rator
Migrate datastore iterator to use base iterator class
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers
@theacodestheacodestheacodes left review comments
@daspecsterdaspecsterdaspecster approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@tseavertseaver

Labels

api: coreapi: datastoreIssues related to the Datastore API.cla: yesThis 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@theacodes@daspecster@tseaver@googlebot