-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
#761: Added ZADD options implementation - XX|NX|CH|INCR| LT|GT according to Redis source code #1262
#761: Added ZADD options implementation - XX|NX|CH|INCR| LT|GT according to Redis source code #1262
Conversation
@JyotinderSingh Can you please review the PR, thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this contribution @rushabhk04! This is a complicated feature with a lot of different permutations. Thanks for the exhaustive tests.
I have left a few comments.
38e3eec
to
dc4bdde
Compare
@JyotinderSingh , I have updated the changes. Can you please re-review it and merge it? Also, please let me know if any questions or suggestions. Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this @rushabhk04! The changes look good.
I have implemented Redis's option parsing for the ZADD command. Additionally, I wrote unit and integration test cases to cover the functionality.
@apoorvyadav1111 Please review this and let me know if any modifications are needed. Thank you!
I added the evaluated command logic, which handles ZADD based on option flags and operates according to the Redis source code. I also created the zset_test file for HTTP, web socket, and resp, which contains test cases for the command with various combinations of options.
Fixes #761
Closed PR related to the above #1138
The existing open PR for this issue is #1205 (the reason I have created a new PR is because of a messed up history in PR 1138).
Will put the documents in the other thread to net mess things up!
Thanks for understanding and assistance!