Merge pull request #838 from fintura/consistent-commands

Consistent commands arguments. All commands are from now on behaving the same no matter if they are on multi or no and they all take an array as either the first or second argument.

Fixes #686 #369 #422 #390 and Closes #634

Author: BridgeAR

.jshintrc

@@ -16,6 +16,9 @@
     "node": true, // Enable globals available when code is running inside of the NodeJS runtime environment.
     "mocha": true,
 
+    // Relaxing options
+    "boss": true, // Accept things like `while (command = keys.shift()) { ... }`
+
     "overrides": {
         "examples/*.js": {
             "unused": false

index.js

@@ -863,8 +863,16 @@ RedisClient.prototype.end = function () {
 function Multi(client, args) {
     this._client = client;
     this.queue = [["multi"]];
+    var command, tmp_args;
     if (Array.isArray(args)) {
-        this.queue = this.queue.concat(args);
+        while (tmp_args = args.shift()) {
+            command = tmp_args.shift();
+            if (Array.isArray(command)) {
+                this[command[0]].apply(this, command.slice(1).concat(tmp_args));
+            } else {
+                this[command].apply(this, tmp_args);
+            }
+        }
     }
 }
 
@@ -878,16 +886,32 @@ commands.forEach(function (fullCommand) {
         return;
     }
 
-    RedisClient.prototype[command] = function (args, callback) {
-        if (Array.isArray(args)) {
-            return this.send_command(command, args, callback);
+    RedisClient.prototype[command] = function (key, arg, callback) {
+        if (Array.isArray(key)) {
+            return this.send_command(command, key, arg);
+        }
+        if (Array.isArray(arg)) {
+            arg.unshift(key);
+            return this.send_command(command, arg, callback);
         }
         return this.send_command(command, to_array(arguments));
     };
     RedisClient.prototype[command.toUpperCase()] = RedisClient.prototype[command];
 
-    Multi.prototype[command] = function () {
-        this.queue.push([command].concat(to_array(arguments)));
+    Multi.prototype[command] = function (key, arg, callback) {
+        if (Array.isArray(key)) {
+            if (arg) {
+                key.push(arg);
+            }
+            this.queue.push([command].concat(key));
+        } else if (Array.isArray(arg)) {
+            if (callback) {
+                arg.push(callback);
+            }
+            this.queue.push([command, key].concat(arg));
+        } else {
+            this.queue.push([command].concat(to_array(arguments)));
+        }
         return this;
     };
     Multi.prototype[command.toUpperCase()] = Multi.prototype[command];
@@ -919,70 +943,63 @@ RedisClient.prototype.auth = RedisClient.prototype.AUTH = function (pass, callba
     }
 };
 
-RedisClient.prototype.hmget = RedisClient.prototype.HMGET = function (arg1, arg2, arg3) {
-    if (Array.isArray(arg2) && typeof arg3 === "function") {
-        return this.send_command("hmget", [arg1].concat(arg2), arg3);
-    } else if (Array.isArray(arg1) && typeof arg2 === "function") {
-        return this.send_command("hmget", arg1, arg2);
-    } else {
-        return this.send_command("hmget", to_array(arguments));
+RedisClient.prototype.hmset = RedisClient.prototype.HMSET = function (key, args, callback) {
+    var field, tmp_args;
+    if (Array.isArray(key)) {
+        return this.send_command("hmset", key, args);
     }
-};
-
-RedisClient.prototype.hmset = RedisClient.prototype.HMSET = function (args, callback) {
-    var tmp_args, tmp_keys, i, il, key;
-
     if (Array.isArray(args)) {
-        return this.send_command("hmset", args, callback);
-    }
-
-    args = to_array(arguments);
-    if (typeof args[args.length - 1] === "function") {
-        callback = args[args.length - 1];
-        args.length -= 1;
-    } else {
-        callback = null;
+        return this.send_command("hmset", [key].concat(args), callback);
     }
-
-    if (args.length === 2 && (typeof args[0] === "string" || typeof args[0] === "number") && typeof args[1] === "object") {
+    if (typeof args === "object") {
         // User does: client.hmset(key, {key1: val1, key2: val2})
         // assuming key is a string, i.e. email address
 
         // if key is a number, i.e. timestamp, convert to string
-        if (typeof args[0] === "number") {
-            args[0] = args[0].toString();
+        // TODO: This seems random and no other command get's the key converted => either all or none should behave like this
+        if (typeof key !== "string") {
+            key = key.toString();
         }
-
-        tmp_args = [ args[0] ];
-        tmp_keys = Object.keys(args[1]);
-        for (i = 0, il = tmp_keys.length; i < il ; i++) {
-            key = tmp_keys[i];
-            tmp_args.push(key);
-            tmp_args.push(args[1][key]);
+        tmp_args = [key];
+        var fields = Object.keys(args);
+        while (field = fields.shift()) {
+            tmp_args.push(field, args[field]);
         }
-        args = tmp_args;
+        return this.send_command("hmset", tmp_args, callback);
     }
-
-    return this.send_command("hmset", args, callback);
+    return this.send_command("hmset", to_array(arguments));
 };
 
-Multi.prototype.hmset = Multi.prototype.HMSET = function () {
-    var args = to_array(arguments), tmp_args;
-    if (args.length >= 2 && typeof args[0] === "string" && typeof args[1] === "object") {
-        tmp_args = [ "hmset", args[0] ];
-        Object.keys(args[1]).map(function (key) {
-            tmp_args.push(key);
-            tmp_args.push(args[1][key]);
-        });
-        if (args[2]) {
-            tmp_args.push(args[2]);
+Multi.prototype.hmset = Multi.prototype.HMSET = function (key, args, callback) {
+    var tmp_args, field;
+    if (Array.isArray(key)) {
+        if (args) {
+            key.push(args);
+        }
+        tmp_args = ['hmset'].concat(key);
+    } else if (Array.isArray(args)) {
+        if (callback) {
+            args.push(callback);
+        }
+        tmp_args = ['hmset', key].concat(args);
+    } else if (typeof args === "object") {
+        tmp_args = ["hmset", key];
+        if (typeof key !== "string") {
+            key = key.toString();
+        }
+        var fields = Object.keys(args);
+        while (field = fields.shift()) {
+            tmp_args.push(field);
+            tmp_args.push(args[field]);
+        }
+        if (callback) {
+            tmp_args.push(callback);
         }
-        args = tmp_args;
     } else {
-        args.unshift("hmset");
+        tmp_args = to_array(arguments);
+        tmp_args.unshift("hmset");
     }
-
-    this.queue.push(args);
+    this.queue.push(tmp_args);
     return this;
 };
 

test/commands/blpop.spec.js

@@ -33,6 +33,16 @@ describe("The 'blpop' method", function () {
                 });
             });
 
+            it('pops value immediately if list contains values using array notation', function (done) {
+                bclient = redis.createClient.apply(redis.createClient, args);
+                client.rpush(["blocking list", "initial value"], helper.isNumber(1));
+                bclient.blpop(["blocking list", 0], function (err, value) {
+                    assert.strictEqual(value[0], "blocking list");
+                    assert.strictEqual(value[1], "initial value");
+                    return done(err);
+                });
+            });
+
             it('waits for value if list is not yet populated', function (done) {
                 bclient = redis.createClient.apply(redis.createClient, args);
                 bclient.blpop("blocking list 2", 5, function (err, value) {

test/commands/client.spec.js

@@ -37,8 +37,17 @@ describe("The 'client' method", function () {
                     });
                 });
 
+                it("lists connected clients when invoked with array syntax on client", function (done) {
+                    client.multi().client(["list"]).exec(function(err, results) {
+                        assert(pattern.test(results[0]), "expected string '" + results + "' to match " + pattern.toString());
+                        return done();
+                    });
+                });
+
                 it("lists connected clients when invoked with multi's array syntax", function (done) {
-                    client.multi().client("list").exec(function(err, results) {
+                    client.multi([
+                        ['client', 'list']
+                    ]).exec(function(err, results) {
                         assert(pattern.test(results[0]), "expected string '" + results + "' to match " + pattern.toString());
                         return done();
                     });

test/commands/dbsize.spec.js

@@ -71,7 +71,7 @@ describe("The 'dbsize' method", function () {
                     var oldSize;
 
                     beforeEach(function (done) {
-                        client.dbsize([], function (err, res) {
+                        client.dbsize(function (err, res) {
                             helper.isType.number()(err, res);
                             assert.strictEqual(res, 0, "Initial db size should be 0");
 

test/commands/del.spec.js

@@ -36,6 +36,20 @@ describe("The 'del' method", function () {
                 client.get('apple', helper.isNull(done));
             });
 
+            it('allows multiple keys to be deleted with the array syntax', function (done) {
+                client.mset('foo', 'bar', 'apple', 'banana');
+                client.del(['foo', 'apple'], helper.isNumber(2));
+                client.get('foo', helper.isNull());
+                client.get('apple', helper.isNull(done));
+            });
+
+            it('allows multiple keys to be deleted with the array syntax and no callback', function (done) {
+                client.mset('foo', 'bar', 'apple', 'banana');
+                client.del(['foo', 'apple']);
+                client.get('foo', helper.isNull());
+                client.get('apple', helper.isNull(done));
+            });
+
             afterEach(function () {
                 client.end();
             });

test/commands/exits.spec.js

@@ -24,6 +24,11 @@ describe("The 'exits' method", function () {
                 client.EXISTS('foo', helper.isNumber(1, done));
             });
 
+            it('returns 1 if the key exists with array syntax', function (done) {
+                client.set('foo', 'bar');
+                client.EXISTS(['foo'], helper.isNumber(1, done));
+            });
+
             it('returns 0 if the key does not exist', function (done) {
                 client.exists('bar', helper.isNumber(0, done));
             });

test/commands/expire.spec.js

@@ -20,6 +20,14 @@ describe("The 'expire' method", function () {
             });
 
             it('expires key after timeout', function (done) {
+                client.set(['expiry key', 'bar'], helper.isString("OK"));
+                client.EXPIRE("expiry key", "1", helper.isNumber(1));
+                setTimeout(function () {
+                    client.exists(["expiry key"], helper.isNumber(0, done));
+                }, 1100);
+            });
+
+            it('expires key after timeout with array syntax', function (done) {
                 client.set(['expiry key', 'bar'], helper.isString("OK"));
                 client.EXPIRE(["expiry key", "1"], helper.isNumber(1));
                 setTimeout(function () {

test/commands/flushdb.spec.js

@@ -58,36 +58,50 @@ describe("The 'flushdb' method", function () {
                 describe("when there is data in Redis", function () {
 
                     beforeEach(function (done) {
-                        var end = helper.callFuncAfter(function () {
-                            client.flushdb(helper.isString("OK", done));
-                        }, 2);
-                        client.mset(key, uuid.v4(), key2, uuid.v4(), helper.isNotError(end));
+                        client.mset(key, uuid.v4(), key2, uuid.v4(), helper.isNotError());
                         client.dbsize([], function (err, res) {
                             helper.isType.positiveNumber()(err, res);
                             assert.equal(res, 2, 'Two keys should have been inserted');
-                            end();
+                            done();
                         });
                     });
 
                     it("deletes all the keys", function (done) {
-                        client.mget(key, key2, function (err, res) {
-                            assert.strictEqual(null, err, "Unexpected error returned");
-                            assert.strictEqual(true, Array.isArray(res), "Results object should be an array.");
-                            assert.strictEqual(2, res.length, "Results array should have length 2.");
-                            assert.strictEqual(null, res[0], "Redis key should have been flushed.");
-                            assert.strictEqual(null, res[1], "Redis key should have been flushed.");
-                            done(err);
+                        client.flushdb(function(err, res) {
+                            assert.equal(res, 'OK');
+                            client.mget(key, key2, function (err, res) {
+                                assert.strictEqual(null, err, "Unexpected error returned");
+                                assert.strictEqual(true, Array.isArray(res), "Results object should be an array.");
+                                assert.strictEqual(2, res.length, "Results array should have length 2.");
+                                assert.strictEqual(null, res[0], "Redis key should have been flushed.");
+                                assert.strictEqual(null, res[1], "Redis key should have been flushed.");
+                                done(err);
+                            });
                         });
                     });
 
                     it("results in a db size of zero", function (done) {
-                        client.dbsize([], function (err, res) {
-                            helper.isNotError()(err, res);
-                            helper.isType.number()(err, res);
-                            assert.strictEqual(0, res, "Flushing db should result in db size 0");
-                            done();
+                        client.flushdb(function(err, res) {
+                            client.dbsize([], function (err, res) {
+                                helper.isNotError()(err, res);
+                                helper.isType.number()(err, res);
+                                assert.strictEqual(0, res, "Flushing db should result in db size 0");
+                                done();
+                            });
                         });
                     });
+
+                    it("results in a db size of zero without a callback", function (done) {
+                        client.flushdb();
+                        setTimeout(function(err, res) {
+                            client.dbsize([], function (err, res) {
+                                helper.isNotError()(err, res);
+                                helper.isType.number()(err, res);
+                                assert.strictEqual(0, res, "Flushing db should result in db size 0");
+                                done();
+                            });
+                        }, 25);
+                    });
                 });
             });
         });

test/commands/get.spec.js

@@ -70,6 +70,21 @@ describe("The 'get' method", function () {
                             done(err);
                         });
                     });
+
+                    it("gets the value correctly with array syntax and the callback being in the array", function (done) {
+                        client.GET([key, function (err, res) {
+                            helper.isString(value)(err, res);
+                            done(err);
+                        }]);
+                    });
+
+                    it("should not throw on a get without callback (even if it's not useful", function (done) {
+                        client.GET(key);
+                        client.on('error', function(err) {
+                            throw err;
+                        });
+                        setTimeout(done, 50);
+                    });
                 });
 
                 describe("when the key does not exist in Redis", function () {