Implemented retry.wrap for #6, also added tests and documentaion #7

Closed
wants to merge 1 commit into
from

Projects

None yet

3 participants

@kof
kof commented Jan 31, 2013

No description provided.

@tim-kos
Owner
tim-kos commented Jun 11, 2014

Hey kof,

sorry for the long delay on this one. Can you please pull the original master and resolve the conflicts and then resubmit the pr?

Thank you!

@kof
kof commented Jun 11, 2014

hi,

done.

P.S: yes, your latency is sensational :)

@tim-kos tim-kos commented on the diff Jun 24, 2014
lib/retry.js
+};
+
+exports.wrap = function(obj, options, methods) {
+ if (options instanceof Array) {
+ methods = options;
+ options = null;
+ }
+
+ if (!methods) {
+ methods = [];
+ for (var key in obj) {
@tim-kos
tim-kos Jun 24, 2014 Owner

This should check for typeof key === 'function', so that other properties on the object that are not functions aren't taken as such.

@tim-kos tim-kos commented on the diff Jun 24, 2014
README.md
@@ -100,6 +100,21 @@ To make this a little easier for you, use wolfram alpha to do the calculations:
[article]: http://dthain.blogspot.com/2009/02/exponential-backoff-in-distributed.html
+### retry.wrap(obj, [options], [methodNames])
+
+Wrap all functions of the `obj` with retry. Optionally you can pass operation options and
+an array of method names which need to be wrapped.
+
+```
+retry.wrap(myLib)
+
+retry.wrap(myLib, ['method1', 'method2']);
+
+retry.wrap(myLib, {retries: 3});
+
+retry.wrap(myLib, {retries: 3}, ['method1', 'method2']);
@tim-kos
tim-kos Jun 24, 2014 Owner

The readme should include that the "options" object can take any options that the usual call to retry can take.

@tim-kos tim-kos commented on the diff Jun 24, 2014
lib/retry.js
+ }
+
+ if (!methods) {
+ methods = [];
+ for (var key in obj) {
+ methods.push(key);
+ }
+ }
+
+ for (var i = 0; i < methods.length; i++) {
+ var method = methods[i];
+ var original = obj[method];
+
+ if (typeof original != 'function') {
+ break;
@tim-kos
tim-kos Jun 24, 2014 Owner

Hm okay, you have the check here. But still I think it makes sense to also include it above.

@tim-kos tim-kos commented on the diff Jun 24, 2014
lib/retry.js
+ }
+ }
+
+ for (var i = 0; i < methods.length; i++) {
+ var method = methods[i];
+ var original = obj[method];
+
+ if (typeof original != 'function') {
+ break;
+ }
+
+ obj[method] = function retryWrapper() {
+ var op = exports.operation(options);
+ var args = Array.prototype.slice.call(arguments);
+ var callback = args.pop();
@tim-kos
tim-kos Jun 24, 2014 Owner

This callback is not part of the documentation yet is it?

Actually nevermind, it doesn't have to be as it's the callback of the function on the object that gets wrapped that's called. What if that function does not have a callback? There should be a check that "callback" is actually a function.

@tim-kos tim-kos commented on the diff Jun 24, 2014
test/integration/test-retry-wrap.js
+ var callbackCalled;
+ var lib = {method: function(a, b, callback) {
+ assert.equal(a, 1);
+ assert.equal(b, 2);
+ assert.equal(typeof callback, 'function');
+ callback();
+ }};
+ retry.wrap(lib);
+ lib.method(1, 2, function() {
+ callbackCalled = true;
+ });
+ assert.ok(callbackCalled);
+}());
+
+(function runWrappedWithError() {
+ var callbackCalled;
@tim-kos
tim-kos Jun 24, 2014 Owner

Great that you added tests as well! 👍

@tim-kos
Owner
tim-kos commented Jun 24, 2014

Once you correct these few things I'd be happy to merge this! Thank you for your effort!

@dirkbonhomme

+1 for merge

@tim-kos
Owner
tim-kos commented Jan 7, 2015

Hey @kof did you see my comments above?

@kof
kof commented Jan 9, 2015

Hi @tim-kos. Time has changed. I am not using it actively right now. I would appreciate if someone else takes my commits and implements the changes.

@tim-kos
Owner
tim-kos commented Sep 1, 2015

Okay, I'll take it over myself.

@tim-kos
Owner
tim-kos commented Sep 16, 2015

This has been implemented in 9446e80

Closing this.

@tim-kos tim-kos closed this Sep 16, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment