feat: use generator for stream results by Linchin · Pull Request #926 · googleapis/python-firestore

Linchin · 2024-06-25T00:04:26Z

add an StreamGenerator class for all class methods stream() to return, instead of using the Python built-in generator.
use AsyncStreamGenerator for the async clients

daniel-sanche · 2024-06-25T18:16:48Z

google/cloud/firestore_v1/query.py

        yield QueryPartition(self, start_at, None)
+
+
+class Iterator(page_iterator.Iterator):


Why turn this into a page_iterator? Previously, it just returned a standard python generator

I was using datastore as a reference, but apparently as you noted below, I could just use a class to wrap this method.

google/cloud/firestore_v1/query.py

daniel-sanche · 2024-06-25T18:23:13Z

google/cloud/firestore_v1/query.py

        yield QueryPartition(self, start_at, None)
+
+
+class Iterator(page_iterator.Iterator):


also: I know this is early, but I'd probably prefer to give the iterator a more descriptive name

I tentatively changed the name to StreamIterator. Maybe we can find a better name, such as DocsIterator?

google/cloud/firestore_bundle/bundle.py

google/cloud/firestore_v1/async_query.py

daniel-sanche · 2024-06-25T22:00:45Z

google/cloud/firestore_v1/async_query.py

+    def __aiter__(self):
+        return self._generator
+
+    def __anext__(self):


I think you shouldn't need to implement the next functions, only iter/aiter. But I could be wrong

If we do need anext as well, you can probably just do

def __anext__(self): return self._generator.__anext__()

Thanks, this looks much more succinct! I think we will need to keep the next methods, because it's one of the methods that generator has.

daniel-sanche · 2024-06-25T22:49:22Z

google/cloud/firestore_v1/stream_iterator.py

+        try:
+            return next(self._generator)
+        except StopIteration:
+            return None


One thought: previously, we were returning generator objects. If you decide to implement next, maybe we should turn this into a full generator and implement send and throw too to be safe?

(I don't think these would be used by current users, but it would keep everything consistent)

I feel like I had better implement these, because send and throw are quite useful in coroutines.

If we were to go this route, should we rename the iterator to generator? Are we going too far on this 😂

Yeah, I guess since the old type annotation was Generator, maybe we should make this into a full StreamGenerator class to be consistent (not that the type annotations can be relied on)

daniel-sanche

A couple small comments about imports and docstrings, but LGTM!

daniel-sanche · 2024-07-02T17:14:41Z

google/cloud/firestore_v1/async_aggregation.py

        Returns:
-            async_stream_iterator.AsyncStreamIterator: A generator of the query
-            results.
+            `AsyncStreamGenerator[DocumentSnapshot]`:


Do you need the ` quotes here? Do you know if devsite renders this well?

Honestly I'm not super sure about this. I added the backticks because they are used in many other type annotations (for example). (A quick search shows the backtick makes sphinx create a link in the docs, but I'm not very clear about which documentation standard we are following.) Maybe we can clean this up along with other type annotations in a future PR. WDYT?

daniel-sanche · 2024-07-02T17:15:40Z

google/cloud/firestore_v1/async_aggregation.py

        retry: Optional[retries.AsyncRetry] = gapic_v1.method.DEFAULT,
        timeout: Optional[float] = None,
-    ) -> async_stream_iterator.AsyncStreamIterator:
+    ) -> "AsyncStreamGenerator[DocumentSnapshot]":


Do you need quotes here? It looks like AsyncStreamGenerator is imported

I added the quotes because [DocumentSnapshot] part would cause an error, because AsyncStreamGenerator isn't recognized as a generator in type hints. I wasn't able to find how to do it, so I resorted to using a string. I don't have a strong opinion on this. What do you think?

daniel-sanche · 2024-07-02T17:16:53Z

google/cloud/firestore_v1/async_aggregation.py

 )
-
-from google.cloud.firestore_v1 import async_stream_iterator, transaction
+from google.cloud.firestore_v1.base_document import DocumentSnapshot


Is this just used for type annotations? If so, you can put it in an if TYPE_CHECKING block to avoid importing it in production

feat: use iterator for query results

ea6135a

Linchin added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jun 25, 2024

Linchin assigned daniel-sanche Jun 25, 2024

product-auto-label bot added size: m Pull request size is medium. api: firestore Issues related to the googleapis/python-firestore API. labels Jun 25, 2024

daniel-sanche reviewed Jun 25, 2024

View reviewed changes

use iterator as suggested in comments

a6e734a

product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Jun 25, 2024

remove unnecessary class

5db952f

product-auto-label bot added size: m Pull request size is medium. and removed size: l Pull request size is large. labels Jun 25, 2024

daniel-sanche reviewed Jun 25, 2024

View reviewed changes

google/cloud/firestore_bundle/bundle.py Outdated Show resolved Hide resolved

daniel-sanche reviewed Jun 25, 2024

View reviewed changes

google/cloud/firestore_v1/async_query.py Outdated Show resolved Hide resolved

more iterators

12c3422

product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Jun 25, 2024

daniel-sanche reviewed Jun 25, 2024

View reviewed changes

Linchin added 10 commits June 25, 2024 15:57

undo adding await

3aa6651

address comments

71e454f

undo bundle change

f206fdf

undo bundle change

b80d9a9

cleanups and docstrings

928d710

fix type hint

6dd5f89

unit tests

a4cbb5a

lint

003387a

skip tests with anext for python < 3.10

8eb14e4

lint

0f56945

Linchin marked this pull request as ready for review June 27, 2024 00:21

Linchin added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 1, 2024

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 1, 2024

Linchin added 3 commits July 1, 2024 13:01

sys test debug

6bfee9b

sys test debug

7d10538

undo change for debug

28c8449

Linchin added the owlbot:run Add this label to trigger the Owlbot post processor. label Jul 1, 2024

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jul 1, 2024

daniel-sanche approved these changes Jul 2, 2024

View reviewed changes

address comment

cc0b611

Linchin added the owlbot:run Add this label to trigger the Owlbot post processor. label Jul 3, 2024

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jul 3, 2024

Linchin added kokoro:force-run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels Jul 3, 2024

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jul 3, 2024

yoshi-kokoro removed kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jul 3, 2024

Linchin added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 3, 2024

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 3, 2024

Linchin added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 8, 2024

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 8, 2024

Linchin added kokoro:force-run Add this label to force Kokoro to re-run the tests. and removed kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jul 8, 2024

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 9, 2024

Linchin added 2 commits July 9, 2024 13:39

system test debug

ea17515

undo system test debug code

e1a2f8d

Linchin added the owlbot:run Add this label to trigger the Owlbot post processor. label Jul 9, 2024

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jul 9, 2024

Linchin merged commit 3e5df35 into googleapis:main Jul 9, 2024

release-please bot mentioned this pull request Jul 9, 2024

chore(main): release 2.17.0 #933

Merged

		yield QueryPartition(self, start_at, None)


		class Iterator(page_iterator.Iterator):

Comments

Conversation

Linchin commented Jun 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daniel-sanche left a comment

Choose a reason for hiding this comment

Uh oh!

daniel-sanche Jul 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Linchin commented Jun 25, 2024 •

edited

Loading

daniel-sanche Jul 2, 2024 •

edited

Loading