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

Firestore: Clarify documentation for '{Document,Collection}.on_snapshot'. #9250

Closed
billyrrr opened this issue Sep 18, 2019 · 1 comment · Fixed by #9275
Closed

Firestore: Clarify documentation for '{Document,Collection}.on_snapshot'. #9250

billyrrr opened this issue Sep 18, 2019 · 1 comment · Fixed by #9275
Assignees
Labels
api: firestore Issues related to the Firestore API. type: docs Improvement to the documentation for an API.

Comments

@billyrrr
Copy link

billyrrr commented Sep 18, 2019

on_snapshot defined in DocumentReference documentation is inconsistent with those of Watch and Product Documentation.

In DocumentReference documentation, we have

callback(Callable[[:class:`~google.cloud.firestore.document.DocumentSnapshot`], NoneType]):
a callback to run when a change occurs

and

def on_snapshot(document_snapshot):
doc = document_snapshot
print(u'{} => {}'.format(doc.id, doc.to_dict()))

In Watch documentation, we have

snapshot_callback: Callback method to process snapshots.
Args:
docs (List(DocumentSnapshot)): A callback that returns the
ordered list of documents stored in this snapshot.
changes (List(str)): A callback that returns the list of
changed documents since the last snapshot delivered for
this watch.
read_time (string): The ISO 8601 time at which this
snapshot was obtained.

In Product Documentation, we have

# Create a callback on_snapshot function to capture changes
def on_snapshot(doc_snapshot, changes, read_time):
    for doc in doc_snapshot:
        print(u'Received document snapshot: {}'.format(doc.id))

doc_ref = db.collection(u'cities').document(u'SF')

# Watch the document
doc_watch = doc_ref.on_snapshot(on_snapshot)

The function signature in DocumentReference takes 1 argument, but the function signature in Watch and Product Documentation takes 3 arguments. Are these referring to the same function? If so, the documentation in DocumentReference may be updated to reflect these changes. If not, on_snapshot in Product Documentation may be renamed to a different name than on_snapshot in DocumentReference to be more clear.

Will probably submit a pull request to update documentation in DocumentReference to be consistent with that in Watch.

Thanks!

@IlyaFaer IlyaFaer added the api: firestore Issues related to the Firestore API. label Sep 19, 2019
@yoshi-automation yoshi-automation added the triage me I really want to be triaged. label Sep 19, 2019
@mf2199 mf2199 added type: docs Improvement to the documentation for an API. and removed triage me I really want to be triaged. labels Sep 19, 2019
@HemangChothani HemangChothani self-assigned this Sep 23, 2019
@tseaver tseaver changed the title Firestore: Clarify documentation for on_snapshot Firestore: Clarify documentation for '{Document,Collection}.on_snapshot'. Sep 23, 2019
@tseaver
Copy link
Contributor

tseaver commented Sep 23, 2019

@billyrrr Thanks for the report. The example for Query.on_snapshot is correct:

def on_snapshot(docs, changes, read_time):
for doc in docs:
print(u'{} => {}'.format(doc.id, doc.to_dict()))

while Document.on_snapshot and Collection.on_snapshot show invalid uses. All three need to update the callback argument specification to show the three arguments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: firestore Issues related to the Firestore API. type: docs Improvement to the documentation for an API.
Projects
None yet
6 participants