Convert GetOptions to Source #826
Conversation
Reworks the existing options object into a source parameter.
|
@mikelehen : Make sure this makes sense wrt your optionsoptions proposal. Thanks. |
Firestore/Source/Public/FIRSource.h
Outdated
|
|
||
| /** | ||
| * An enum that configures the behavior of `DocumentReference.getDocument()` | ||
| * and `CollectionReference.getDocuments()`. By providing a source enum the |
There was a problem hiding this comment.
This should probably be Query.getDocuments() ?
Firestore/Source/Public/FIRSource.h
Outdated
| * server, only from the local cache, or attempt the server and fall back to | ||
| * the cache (which is the default). | ||
| * | ||
| * Setting the source to FIRSourceDefault, if online, causes Firestore to try |
There was a problem hiding this comment.
Since we're using Swift syntax above, this should perhaps be Source.default ? [in general we're really inconsistent about Swift vs Obj-C in our comments, but we may as well be consistent within a single comment block. :-)]
There was a problem hiding this comment.
Consistency? Hmm... well I suppose we could try that. Done.
Firestore/Source/Public/FIRSource.h
Outdated
| * | ||
| * FIRSourceServer causes Firestore to avoid the cache (generating an error if | ||
| * a value cannot be retrieved from the server). The cache will be updated if | ||
| * the RPC succeeds. Latency compensation still occurs (implying that if the |
There was a problem hiding this comment.
I'd avoid reference "the RPC" since that's dropping to a lower abstraction layer. Perhaps just "The cache will be updated with the data retrieved from the server."
Firestore/Source/Public/FIRSource.h
Outdated
| * FIRSourceServer causes Firestore to avoid the cache (generating an error if | ||
| * a value cannot be retrieved from the server). The cache will be updated if | ||
| * the RPC succeeds. Latency compensation still occurs (implying that if the | ||
| * cache is more up to date, then it's values will be merged into the results). |
There was a problem hiding this comment.
I'd say something like "(that is, any local pending writes will be visible in the results)" as "is more up to date" isn't quite accurate. FWIW I think I've commented on this verbiage somewhere else (the JS PR?) and I'd try to keep them in sync so you may want to look at that as well.
There was a problem hiding this comment.
Done.
The JS one is certainly out-of-date, so I'll consider this one the "master" and will update the js one to match.
There was a problem hiding this comment.
Please still do a scan over the feedback on the JS one to see if anything should be applied to this PR...
There was a problem hiding this comment.
Ah, I see what you mean. Backported.
Firestore/Source/Public/FIRSource.h
Outdated
| * | ||
| * FIRSourceCache causes Firestore to immediately return a value from the | ||
| * cache, ignoring the server completely (implying that the returned value may | ||
| * be stale with respect to the value on the server.) For a single document, |
There was a problem hiding this comment.
nit: Period should be outside the paren. :-)
Firestore/Source/Public/FIRSource.h
Outdated
| * FIRSourceCache causes Firestore to immediately return a value from the | ||
| * cache, ignoring the server completely (implying that the returned value may | ||
| * be stale with respect to the value on the server.) For a single document, | ||
| * the get will fail if the document doesn't exist. |
There was a problem hiding this comment.
"doesn't exist" should be "isn't in the cache" or similar, since we don't know if it exists.
Firestore/Source/Public/FIRSource.h
Outdated
| * be stale with respect to the value on the server.) For a single document, | ||
| * the get will fail if the document doesn't exist. | ||
| */ | ||
| typedef NS_ENUM(NSUInteger, FIRSource) { |
There was a problem hiding this comment.
Did we explicitly decide to change from FIRGetSource to FIRSource? I may be forgetting a discussion, but I think I'd be inclined to keep it as FIRGetSource (less likely to conflict with other Firebase naming in the future, and since you can omit it in Swift it doesn't affect usability).
There was a problem hiding this comment.
For objc? I don't think we did. I "back-ported" the Source from swift to FIRSource. I've changed it to FIRGetSource (and left swift as Source.)
|
Update changelog. |
Reworks the existing options object into a source parameter.