Skip to content

Redis EVAL and EVALSHA fixes and tests.#2555

Closed
unknownnf wants to merge 8 commits intofacebook:masterfrom
unknownnf:patch-1
Closed

Redis EVAL and EVALSHA fixes and tests.#2555
unknownnf wants to merge 8 commits intofacebook:masterfrom
unknownnf:patch-1

Conversation

@unknownnf
Copy link
Contributor

Fixed:

  • EVAL and EVALSHA need numKeys as first argument after script / SHA otherwise ERR is thrown - http://redis.io/commands/EVAL
  • When calling an aliased method with __call() only $args should be passed as arguments.

Added:

  • Tests for eval(), evalSha() and script()
  • Aliases for eval and evaluate to _eval, evalSha and evaluateSha to _evalSha
  • Function doProcessVariantResponse() to process multibulk responses.

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

@unknownnf
Copy link
Contributor Author

Signed.

@JoelMarcey JoelMarcey self-assigned this May 1, 2014
@JoelMarcey
Copy link
Contributor

@unknownnf Thanks! I am going to put this PR up for review internally right now.

@unknownnf unknownnf changed the title Redis EVAL and EVALSHA fixes and tests. Redis fixes and tests. May 3, 2014
@unknownnf
Copy link
Contributor Author

I'm done with this PR for the moment since it's getting pretty big, I'll do other tests/fixes after it gets merged and I have more time.

@JoelMarcey
Copy link
Contributor

@unknownnf Oh wow, you have added more commits to this PR. I have already put this PR for review up until and including 0f6549f.

Maybe the other 6 commits you have added since then can go in a separate PR? I suppose I could bring them in and squash them, but, like you said, we are already pretty big as it is.

What do you think?

Alias evaluate, evaluateSha and evalSha, rename evalSha to _evalSha for consistency.
More complete tests, covering _eval and _evalSha aliases as well as script.
@unknownnf
Copy link
Contributor Author

Allright, I've removed the extra commits and cleaned up the current PR, updated description. I'll prepare another PR for the other tests and fixes.

@unknownnf unknownnf changed the title Redis fixes and tests. Redis EVAL and EVALSHA fixes and tests. May 3, 2014
@JoelMarcey JoelMarcey closed this in bb65484 May 6, 2014
@unknownnf unknownnf deleted the patch-1 branch May 6, 2014 18:46
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