-
Notifications
You must be signed in to change notification settings - Fork 181
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
Support NX/XX/CH flags in ZADD command #247
Conversation
Add support for the new `NX`, `XX`, and `CH` flags in the `ZADD` command, which can only be provided with `redis-py` version 3 installed. This fixes a subset of what's filed in jamesls#232. `INCR` flag will be implemented in a different pull request, as it's completely different to handle.
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! Looks good, just a few suggestions for strengthening the tests.
test_fakeredis.py
Outdated
self.assertEqual(self.zadd('foo', {'four': 4, 'three': 3, 'zero': 0}, xx=True), 0) | ||
self.assertEqual(self.redis.zrange('foo', 0, -1), [b'three', b'four']) | ||
self.assertEqual(self.zadd('foo', {'two': 2, 'one': 1}, xx=True), 0) | ||
self.assertEqual(self.redis.zrange('foo', 0, -1), [b'three', b'four']) |
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.
Add a check that the scores do get updated.
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.
I refactored the tests to use zrange
with scores, and made them a bit more programmatic, to improve readability. Let me know what you think!
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.
I'm happy with that approach. What do you think about using a namedtuple
rather than a dict with fixed keys to store the update information?
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.
Updated to use namedtuples, thanks for the suggestion! Let me know if naming works for you :)
Looks good. Thanks for the contribution! |
Add support for the new
NX
,XX
, andCH
flags in theZADD
command, which can only be provided withredis-py
version 3 installed.This fixes a subset of what's filed in #232.
INCR
flag will be implemented in a different pull request, as it's completely different to handle.