-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Making DocumentSnapshot.data() nullable #553
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with nits. Also please add a changelog entry.
|
||
@import FirebaseFirestore; | ||
|
||
#import <FirebaseFirestore/FIRCollectionReference.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised these are required. @import FirebaseFirestore;
should have done this for you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed. App Code auto-adds them. I need to be more careful here :/
@end | ||
|
||
/** | ||
* A `FIRQueryDocumentSnapshot` contains data read from a document in your Firestore database. The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps "contains data read from a document in your Firestore database as a part of a query."
Because these types are related we should add clarifying text here that describes when the two types are used, i.e. DocumentSnapshot
for single document gets and listens, QueryDocumentSnapshot
for query/collection gets/listens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the comments. Let me know what you think since I need to port them to the other platforms.
Comments addressed. I left the Is this an update we need to make? Otherwise, I will remove it. |
Comments LGTM, please squash and merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Making DocumentSnapshot.data() nullable
This PR makes DocumentSnapshot.data() nullable and introduces a non-nullable QueryDocumentSnapshot type.