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.

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

datastore: fix - mark transactions as finalized. #250

Conversation

stephenplusplus
Copy link
Contributor

The recent API refactoring removed the functionality where save and delete operations mark the transaction as finalized. This brings it back.

@ryanseys
Copy link
Contributor

ryanseys commented Oct 8, 2014

Oops. Do we have to wrap the method or can we detect that it's a transaction by looking for this.id?

@stephenplusplus
Copy link
Contributor Author

Oh, good call. We can finalize from datastore request if you think that's
clearer

On Wednesday, October 8, 2014, Ryan Seys notifications@github.com wrote:

Oops. Do we have to wrap the method or can we detect that it's a
transaction by looking for this.id?


Reply to this email directly or view it on GitHub
#250 (comment)
.

@ryanseys
Copy link
Contributor

ryanseys commented Oct 8, 2014

I'd prefer all the logic to be there, yes. Much easier to follow it that way.

@stephenplusplus stephenplusplus force-pushed the spp--datastore-finalize-transactions branch 2 times, most recently from e59a61f to cc519d4 Compare October 9, 2014 00:14
@stephenplusplus stephenplusplus force-pushed the spp--datastore-finalize-transactions branch from cc519d4 to c79a52d Compare October 9, 2014 00:14
@stephenplusplus
Copy link
Contributor Author

Updated with the new plan.

this.createRequest_('commit', req, res, callback);
this.createRequest_('commit', req, res, function(err) {
if (!err && this.id) {
this.isFinalized = true;

This comment was marked as spam.

This comment was marked as spam.

@ryanseys
Copy link
Contributor

ryanseys commented Oct 9, 2014

Just worried if the transaction isn't finalized when a response is received, that could have unexpected side-effects. Other than that, this looks good to me :)

@silvolu
Copy link
Contributor

silvolu commented Oct 9, 2014

Can we test it somehow? Having a test would have avoided the regression.

@stephenplusplus
Copy link
Contributor Author

@silvolu I had the same thought. I figured I'd let this PR go, then attack a re-factor of datastore tests and add it at that time, as well as anything else that's missing. After we put the shared logic into request.js, our test files aren't split to match, so functionality testing is a bit criss-crossed atm.

@silvolu
Copy link
Contributor

silvolu commented Oct 9, 2014

Ok, I'll merge this, then let's wait for the tests to be fixed before releasing 0.8.1

silvolu added a commit that referenced this pull request Oct 9, 2014
…transactions

datastore: fix - mark transactions as finalized.
@silvolu silvolu merged commit c528eed into googleapis:master Oct 9, 2014
@ryanseys
Copy link
Contributor

ryanseys commented Oct 9, 2014

The tests aren't failing so curious what "vector" you're going to take to approach fixing them? Asking for a friend 😉

@silvolu
Copy link
Contributor

silvolu commented Oct 9, 2014

Sorry, poor choice of word :)
'Fixing' as in split the files to reflect the new structure and add coverage for functionalities that are not tested (like this one).

@ryanseys
Copy link
Contributor

ryanseys commented Oct 9, 2014

Makes sense. My apologies for assuming they were sufficient as-is :) Let me know if I can help with the transition.

chingor13 pushed a commit that referenced this pull request Aug 22, 2022
BREAKING CHANGE: removes projectPath helper, instead use "projects/${project}".
sofisl pushed a commit that referenced this pull request Sep 15, 2022
sofisl pushed a commit that referenced this pull request Sep 27, 2022
* updated CHANGELOG.md

* updated package.json

* updated samples/package.json
sofisl pushed a commit that referenced this pull request Oct 11, 2022
Fixes #234
Fixes #250

Addressed comments.
sofisl pushed a commit that referenced this pull request Nov 11, 2022
…ript generator. (#250)

Also removing the explicit generator tag for the IAMPolicy mixin for the kms and pubsub APIS as the generator will now read it from the .yaml file.

PiperOrigin-RevId: 385101839

Source-Link: googleapis/googleapis@80f4042

Source-Link: googleapis/googleapis-gen@d3509d2
sofisl pushed a commit that referenced this pull request Nov 11, 2022
The library is regenerated with gapic-generator-typescript v1.3.1.

Committer: @alexander-fenster
PiperOrigin-RevId: 372468161

Source-Link: googleapis/googleapis@75880c3

Source-Link: googleapis/googleapis-gen@77b1804
sofisl pushed a commit that referenced this pull request Nov 11, 2022
- [ ] Regenerate this pull request now.

PiperOrigin-RevId: 474338479

Source-Link: googleapis/googleapis@d5d35e0

Source-Link: googleapis/googleapis-gen@efcd3f9
Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiZWZjZDNmOTM5NjJhMTAzZjY4ZjAwM2UyYTFlZWNkZTZmYTIxNmEyNyJ9
sofisl pushed a commit that referenced this pull request Nov 11, 2022
* chore(main): release 3.0.3

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
sofisl pushed a commit that referenced this pull request Nov 11, 2022
BREAKING CHANGE: removes projectPath helper, instead use "projects/${project}".
sofisl pushed a commit that referenced this pull request Nov 11, 2022
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
sofisl pushed a commit that referenced this pull request Nov 11, 2022
This PR was generated using Autosynth. 🌈

Synth log will be available here:
https://source.cloud.google.com/results/invocations/35a31f24-4249-4b7d-9c51-7f63c556390c/targets

- [ ] To automatically regenerate this PR, check this box.

Source-Link: googleapis/synthtool@0c868d4
sofisl pushed a commit that referenced this pull request Nov 30, 2022
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
sofisl pushed a commit that referenced this pull request Jan 24, 2023
sofisl pushed a commit that referenced this pull request Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants