-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Adding support for ZMPOP command #2408
Conversation
I don't know how the test passes, I run this PR test and I get the error:
Oh, See #2411 |
…/go-redis into ab/support-for-ZMPOP
Closing till I fix and debug the tests , will re-open once it is done |
… ab/support-for-ZMPOP
…/go-redis into ab/support-for-ZMPOP
Hey @monkey92t I worked on this and learned from your implementation of LMPOP yesterday, I wrote tests from the examples in https://redis.io/commands/zmpop/ |
@monkey92t @chayim Should I rename the command to ZArrayWithKeyCmd ( matching the standard from |
@monkey92t Can you please review this if possible |
@SoulPancake overall it looks good. My comment was literally only a nit. CI failures don't appear related. @monkey92t or @vladvildanov you have more experience here. Does this frequently happen with CI in this project? |
For the name |
For commands with complex parameters, we should add some comments and usage examples. |
Thanks everyone for their valuable comments and suggestions, Learning a lot @chayim @monkey92t @vladvildanov |
Adding support for ZMPOP