Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: split insert, update, upsert #693

Merged
merged 17 commits into from
Sep 1, 2020
Merged

fix: split insert, update, upsert #693

merged 17 commits into from
Sep 1, 2020

Conversation

AVaksman
Copy link
Contributor

@AVaksman AVaksman commented Jun 29, 2020 •
edited
Loading

  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #540

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 29, 2020
@codecov
Copy link

codecov bot commented Jul 2, 2020 •
edited
Loading

Codecov Report

Merging #693 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #693      +/-   ##
==========================================
+ Coverage   98.85%   98.86%   +0.01%     
==========================================
  Files          10       10              
  Lines        7244     7316      +72     
  Branches      479      440      -39     
==========================================
+ Hits         7161     7233      +72     
  Misses         81       81              
  Partials        2        2
Impacted Files Coverage Δ
src/request.ts 100.00% <ø> (ø)
src/index.ts 99.80% <100.00%> (+0.09%) ⬆️
src/transaction.ts 99.61% <100.00%> (+0.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3362f3c...46b683c. Read the comment docs.

@AVaksman AVaksman marked this pull request as ready for review July 2, 2020 03:20
@AVaksman AVaksman added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 2, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 2, 2020
@AVaksman AVaksman requested a review from a team as a code owner July 6, 2020 20:10
@sofisl sofisl changed the title fix: split insert, update, upsert typeo Jul 9, 2020
@sofisl sofisl changed the title typeo fix: split insert, update, upsert Jul 9, 2020
BenWhitehead
BenWhitehead reviewed Jul 29, 2020
View reviewed changes
Copy link
Contributor

@BenWhitehead BenWhitehead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In concept I think this is a great idea to clean up the confusion outlined in #540.

I however do not know typescript well enough to evaluate the actual code change, so I'll defer code approval to them.

@stephenplusplus
Copy link
Contributor

@AVaksman could you provide a summary of the changes in the description, and maybe a before/after?

@AVaksman
Copy link
Contributor Author

AVaksman commented Aug 3, 2020

Currently functions insert, update, upsert and save are implemented in the base class DatastoreRequest as asynchronous functions. The functions are then inherited by Datastore and Transaction classes where Datastore functions are meant to be asynchronous, and execute an RPC each, but Transaction functions not. The Transaction functions are designed to only prep the entity and push to modifiedEntities_ array while awaiting Transaction.commit().
In this PR the aforementioned functions are moved from DatastoreRequest class to Datastore and Transaction` classes with the intended implementation in each of them.

@product-auto-label product-auto-label bot added the api: datastore Issues related to the googleapis/nodejs-datastore API. label Aug 21, 2020
@AVaksman
Copy link
Contributor Author

AVaksman commented Sep 1, 2020

Gentle ping

stephenplusplus
stephenplusplus suggested changes Sep 1, 2020
View reviewed changes
src/index.ts Outdated
* // blobs, and lists.
* //
* // Notice that we are providing an incomplete key. After saving, the
* original
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formatting issue here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed, thanks.

src/index.ts Outdated
* // Save a single entity using a provided name instead of auto-generated ID.
* //
* // Here we are providing a key with name instead of an ID. After saving,
* the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formatting issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed, thanks.

src/index.ts Outdated
* // Here we are providing a key with name instead of an ID. After saving,
* the
* // original Key object used to save will be updated to contain the path
* with
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formatting issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed, thanks.

src/index.ts
* };
*
* datastore.save(entity, (err, apiResponse) => {});
* //-
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New line above here, please.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added, thanks.

@AVaksman
Copy link
Contributor Author

AVaksman commented Sep 1, 2020

Thank you!

@AVaksman AVaksman merged commit fa5faac into googleapis:master Sep 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BenWhitehead BenWhitehead BenWhitehead left review comments

@stephenplusplus stephenplusplus stephenplusplus approved these changes

Assignees
No one assigned
Labels
api: datastore Issues related to the googleapis/nodejs-datastore API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Transaction API returns promises that are never resolved
7 participants
@AVaksman @stephenplusplus @BenWhitehead @googlebot @yoshi-kokoro @bcoe @JustinBeckwith