Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

EVAL: paramaters as an array support #368

Closed
wants to merge 2 commits into from

3 participants

@dmoena

EVAL function overwrite on index.js (:1068) caused lost of support to handle parameters as an array ( [ param1, param2, ... , paramN ], callback), since assumes all of them will be passed on extended format ( param1, param2, ... , paramN, callback ).

I added a simple validation to make it compatible. Also, updated example code to probe and show how this two formats works fine now.

Ready to merge checklist

  • test(s) in test.js
  • tests will fail without the PR, but succeed once applied
  • docs, if applicable
  • merges cleanly
  • coding style (4-space indents, looks similar to other code)
@DTrejo
Collaborator

Looks good, could you add a test to tests.js? I added a little checklist so you can see how close this is to being merged.

@dmoena

Hey, while working on the test, I got some of them failing. After checking redis' site, I know what the problem is.

Basically,"Errors inside a transaction" at http://redis.io/topics/transactions states that version 2.6.5+ handles errors on a different way. Now, if you get an error, the whole transaction fails, as opposite to previous (and tested) behavior.

I already made some changes, but while doing that, I start to think that you might want to deal with this in a different way (I basically migrate all failing tests to make them 2.6.5+ compatible). Also, you might want to have this in a different pull request...

Let me know your decision and how can I help.

@brycebaril
Collaborator

@dmoena I have a fix for making the tests 2.6.5+ compatible already, will be submitted as a PR soon. In the meantime feel free to comment out MULTI_1 and MULTI_2 tests in your workspace.

@dmoena

I commented failing tests locally, just to add mine. Comments were removed before commit.

@DTrejo DTrejo closed this pull request from a commit
@DTrejo DTrejo EVAL: allow parameters as an array. Close #368.
Signed-off-by: DTrejo <david.daniel.trejo@gmail.com>
938c052
@DTrejo DTrejo closed this in 938c052
@DTrejo
Collaborator

Thank you!
D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 1, 2013
Commits on Feb 19, 2013
This page is out of date. Refresh to see the latest.
Showing with 20 additions and 1 deletion.
  1. +7 −1 examples/eval.js
  2. +4 −0 index.js
  3. +9 −0 test.js
View
8 examples/eval.js
@@ -1,4 +1,4 @@
-var redis = require("./index"),
+var redis = require("../index"),
client = redis.createClient();
redis.debug_mode = true;
@@ -7,3 +7,9 @@ client.eval("return 100.5", 0, function (err, res) {
console.dir(err);
console.dir(res);
});
+
+/* dmoena: this supported format was not working */
+client.eval([ "return 100.5", 0 ], function (err, res) {
+ console.dir(err);
+ console.dir(res);
+});
View
4 index.js
@@ -1074,6 +1074,10 @@ RedisClient.prototype.eval = RedisClient.prototype.EVAL = function () {
callback = args.pop();
}
+ if (Array.isArray(args[0])) {
+ args = args[0];
+ }
+
// replace script source with sha value
var source = args[0];
args[0] = crypto.createHash("sha1").update(source).digest("hex");
View
9 test.js
@@ -281,6 +281,15 @@ tests.EVAL_1 = function () {
assert.strictEqual("d", res[3], name);
});
+ // test {EVAL - parameters in array format gives same result}
+ client.eval(["return {KEYS[1],KEYS[2],ARGV[1],ARGV[2]}", 2, "a", "b", "c", "d"], function (err, res) {
+ assert.strictEqual(4, res.length, name);
+ assert.strictEqual("a", res[0], name);
+ assert.strictEqual("b", res[1], name);
+ assert.strictEqual("c", res[2], name);
+ assert.strictEqual("d", res[3], name);
+ });
+
// prepare sha sum for evalsha cache test
var source = "return redis.call('get', 'sha test')",
sha = crypto.createHash('sha1').update(source).digest('hex');
Something went wrong with that request. Please try again.