Skip to content
This repository has been archived by the owner on Dec 20, 2023. It is now read-only.

problems with the new "isolated" option #110

Merged
merged 4 commits into from
Apr 25, 2014
Merged

problems with the new "isolated" option #110

merged 4 commits into from
Apr 25, 2014

Conversation

cvlmtg
Copy link
Contributor

@cvlmtg cvlmtg commented Apr 24, 2014

this fixes a typo for the new option (called "isolate" in the code but "isolated" in the documentation) and should fix some problems with these new transactions.

Matteo Cavalleri added 3 commits April 24, 2014 12:04
I decided to change "isolate" to "isolated" and not
the other way round because the documentation says
the option is called "isolated"
@@ -583,7 +583,7 @@ var Database = define(null, {
self.__transaction(null, opts, cb).chain(ret.callback, ret.errback);
};
this.__transactionQueue.enqueue(transaction);
return ret.promise();
return ret;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please change this back to ret.promise() this ensures that invoking code cannot resolve the promise.

@doug-martin
Copy link
Contributor

Thanks for the pull request! Other than my comments it looks good once those are addressed I will merge.

@cvlmtg
Copy link
Contributor Author

cvlmtg commented Apr 24, 2014

ok I've reverted those two changes. I'm used to the callback style, so I'm all but an expert with promises...

just a question though: shouldn't line 575 changed into a return too?

doug-martin added a commit that referenced this pull request Apr 25, 2014
problems with the new "isolated" option
@doug-martin doug-martin merged commit 6b2c3ea into C2FO:master Apr 25, 2014
doug-martin added a commit to doug-martin/patio that referenced this pull request Apr 25, 2014
* Update `comb` to `v0.3.0`
* Fixed issues with `savepoints` and update transaction to conform with docs. C2FO#110
@doug-martin doug-martin mentioned this pull request Apr 25, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants