Adding unknownDocument #1948
Adding unknownDocument #1948
Conversation
schmidt-sebastian
force-pushed
the
mrschmidt-emptydocument
branch
from
f572cc6 to
9feb291
Compare
There was a problem hiding this comment.
There's a bunch of style nits, all of which are present in the original too. If you fix them here, you should probably also fix them in the java repo.
The only one that's remotely significant is the naming, since if we fix that later, we'll have to change all the source that uses it.
| @@ -20,14 +20,22 @@ message NoDocument { | |||
| google.protobuf.Timestamp read_time = 2; | |||
| } | |||
|
|
|||
| // Represents either an existing document or the explicitly known absence of a | |||
| // document. | |||
| // Represents either an existing document, the explicitly known absence of a document, or a document | |||
There was a problem hiding this comment.
nit: Don't we line wrap to 80 chars for protos?
There was a problem hiding this comment.
This was written in Android Studio, which wraps at 100. It makes sense to limit at 80 though.
| } | ||
|
|
||
| // `hasCommittedMutations` marks documents that were written to the remote socument store based |
| } | ||
|
|
||
| // `hasCommittedMutations` marks documents that were written to the remote socument store based | ||
| // on a write acknowledgment. These documents are potentially inconsistent with the backend's |
There was a problem hiding this comment.
nitnitnit: double space between 'the' and 'backend'
| // `hasCommittedMutations` marks documents that were written to the remote socument store based | ||
| // on a write acknowledgment. These documents are potentially inconsistent with the backend's | ||
| // copy and use the write's commit version as their document version. | ||
| bool hasCommittedMutations = 4; |
There was a problem hiding this comment.
protos usually use snake_case for fields, i.e. 'has_committed_mutations'.
There was a problem hiding this comment.
Yeah, sorry - we should rename this field.
|
And while we're at it, this file is also missing a copyright header (as are the other files in this directory, though curiously not the protos in other directories.) Maybe fix that too? (Though maybe as a separate PR.) (And apologies for the repeated post-review comments) |
schmidt-sebastian
force-pushed
the
mrschmidt-emptydocument
branch
from
9958fd3 to
0b6ac70
Compare
|
Copyright headers added as well |
| // on a write acknowledgment. These documents are potentially inconsistent with the backend's | ||
| // copy and use the write's commit version as their document version. | ||
| bool hasCommittedMutations = 4; | ||
| // `hasCommittedMutations` marks documents that were written to the remote |
There was a problem hiding this comment.
nit: consider fixing the name in the comment too.
| @@ -22,6 +36,7 @@ message Target { | |||
| // The last snapshot version received from the Watch Service for this target. | |||
| // | |||
| // This is the same value as TargetChange.read_time | |||
| // https://github.com/googleapis/googleapis/blob/master/google/firestore/v1beta1/firestore.proto#L734 | |||
There was a problem hiding this comment.
I don't much care, but this comment could go stale and end up pointing to the wrong thing if firestore.proto is changed in the future.
Solutions:
- Nothing. If it changes, the reader will figure it out.
- Embed the commit hash in the url, i.e. https://github.com/googleapis/googleapis/blob/02111201c4752cdd0d7a5714ec6644f1d0297f51/google/firestore/v1beta1/firestore.proto#L734
- Omit "L734" and let the reader ctrl+f.
There was a problem hiding this comment.
Leaving as is, since this URL exists in the Android repo. I wanted both files to be the same
Updating the iOS Proto to match Android. This is in preparation for the held write acks removal.