Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EVAL: paramaters as an array support #368

Closed
wants to merge 2 commits into from
Closed

Conversation

dmoena
Copy link

@dmoena dmoena commented Feb 1, 2013

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
Copy link
Contributor

DTrejo commented Feb 18, 2013

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
Copy link
Author

dmoena commented Feb 19, 2013

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
Copy link
Contributor

@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
Copy link
Author

dmoena commented Feb 19, 2013

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

@DTrejo DTrejo closed this in 938c052 Feb 24, 2013
@DTrejo
Copy link
Contributor

DTrejo commented Feb 24, 2013

Thank you!
D

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.

None yet

3 participants