Skipping empty write requests #74
Conversation
ghost
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## master #74 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 12 12
Lines 1547 1596 +49
=====================================
+ Hits 1547 1596 +49
Continue to review full report at Codecov.
|
schmidt-sebastian
force-pushed
the
mrschmidt-onetimestamp
branch
from
0712124 to
c05cb11
Compare
schmidt-sebastian
force-pushed
the
mrschmidt-onetimestamp
branch
from
ea9fad9 to
e6c41fc
Compare
There was a problem hiding this comment.
This is a lot of code churn for slightly questionable benefit, but I guess it's worth it to increase our test coverage... in that name though, I'm going slightly light on code review and trusting that our tests are covering this code sufficiently well. I did flag a few things though.
conformance/runner.js
Outdated
| resolve(); | ||
| try { | ||
| assert.deepEqual(request, expected); | ||
| resolve(); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
src/document.js
Outdated
| * turns { foo.bar : foobar } into { foo { bar : foobar }} | ||
| * | ||
| * @private | ||
| * @param {firestore/DocumentReference} ref - The reference to the documen.t |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
src/document.js
Outdated
| return true; | ||
| } | ||
|
|
||
| return isEmptyObject(this._fieldsProto); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
src/write-batch.js
Outdated
| ); | ||
|
|
||
| if (explicitTransaction) { | ||
| request.transaction = explicitTransaction; | ||
| } | ||
|
|
||
| // We store the number of operations that we sent as part of this commit | ||
| // as the user is able to add additional operations afterwards. |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
Addressed comments. I agree that this PR brings very little benefit to Node, but I think the benefit that it brings to the GCloud SDKs as a whole far outweighs the work that has gone into this. |
src/write-batch.js
Outdated
| // We store the number of operations that we sent as part of this commit | ||
| // as the user is able to add additional operations afterwards. | ||
| let opCount = this._writes.length; | ||
| this._committed = true; |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| @@ -294,6 +294,21 @@ describe('batch support', function() { | |||
| }); | |||
| }); | |||
|
|
|||
| it('cannot append to committed batch', function() { | |||
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
…s/nodejs-firestore into mrschmidt-onetimestamp
ghost
removed
the
cla: yes
This is required for a full pass of the conformance test.
This adds fromObject and fromUpdateMap to DocumentTransform, DocumentMask, and DocumentSnapshot and moves some of the existing logic to expand objects into these functions.
This also adds the ability to delete nested fields via merge. set({a : { b: delete}}, merge) was previously not supported, but I needed it for the system test.