-
Notifications
You must be signed in to change notification settings - Fork 149
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
fix: Add BulkWriter #1051
fix: Add BulkWriter #1051
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1051 +/- ##
==========================================
- Coverage 97.58% 97.48% -0.10%
==========================================
Files 25 27 +2
Lines 16015 16860 +845
Branches 1295 1261 -34
==========================================
+ Hits 15628 16436 +808
- Misses 384 420 +36
- Partials 3 4 +1
Continue to review full report at Codecov.
|
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.
This came together nicely. I have three small requests before merging:
- Address the one comment nit in this PR.
- Verify that the output of
yarn docs
doesn't contain any snippets that should be hidden. It might be a good idea to do a general scrub of the documentation thatyarn docs
writes to./docs
. - Change the PR title to make sure this is not a minor version bump and to not overpromise in our release notes. We were not actually able to add a new feature - just a bunch of amazing looking code :)
* .then(res => { | ||
* console.log(`Deleted document at ${res.writeTime}`); | ||
* }); | ||
* await bulkWriter.flush().then(() => { |
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.
Nit: Make this bulkWriter.close()
and remove line 678.
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.
done.
Regenerated the docs and verified that no globals or classes leaked. Thanks so much for helping shepherd this through Sebastian! |
This reverts commit 8c52d47.
Keeping fields private until protos are publicly released.