Supports argument score_cast_func in zrangebyscore() #194
Conversation
fferrara
force-pushed
the
zrangebyscore-new-argument
branch
from
ff3332f
to
fdff224
Compare
fakeredis.py
Outdated
| @@ -1638,11 +1638,13 @@ def zrangebyscore(self, name, min, max, | |||
|
|
|||
| ``withscores`` indicates to return the scores along with the values. | |||
| The return type is a list of (value, score) pairs | |||
|
|
|||
| `score_cast_func`` a callable used to cast the score return value | |||
There was a problem hiding this comment.
Missing a leading backtick (looks like that's inherited from redis-py, but might as well fix it). Same issue on zrevrangebyscore.
test_fakeredis.py
Outdated
| self.redis.zrangebyscore('foo', 2, 3, withscores=True, score_cast_func=round_str), | ||
| expected_with_cast_round | ||
| ) | ||
|
|
There was a problem hiding this comment.
Test looks fine, but can you add a similar test for zrevrangebyscore for completeness.
|
I'll try to add support for |
|
That sounds about right - but I'd suggest just searching for it in redis-py. |
|
Seems like those tests don't pass on real redis. I haven't investigated, but my guess is that redis-py calls the score_cast_func with the raw bytes from the wire, not a float - so you should probably do the same, and then write the tests with a score_cast_func that expects a string (or I'd guess |
fferrara
force-pushed
the
zrangebyscore-new-argument
branch
from
f284900
to
b0eea8d
Compare
|
@bmerry I fixes the tests, they must support strings/bytes indeed. How do you feel about this PR? |
|
It's looking good, but the float/str issue doesn't just affect tests, it needs to be reflected in fakeredis.py too: the float score needs to be encoded as a string before being passed to score_cast_func; and it would be nice if the score_cast_func used in the tests verified that they receive a string (I'd also like to just have one copy of the function, instead of a local copy in each test). |
|
Agreed on the second point. I'm not sure I follow the first one: "the float score needs to be encoded as a string before being passed to score_cast_func". Actually, as we saw with the tests failing, the score is already a |
|
In redis-py it is a bytes object, but as far as I can see in fakeredis it is still a float. So to emulate redis-py, fakeredis will need to turn the float it has into a bytes object before passing it to score_cast_func (which in the default case will turn it back into a float again). |
test_fakeredis.py
Outdated
| @@ -74,6 +74,13 @@ def key_val_dict(size=100): | |||
| for i in range(size)]) | |||
|
|
|||
|
|
|||
| def round_str(x): | |||
| if not hasattr(x, 'decode'): | |||
| raise AssertionError('Cast argument should be bytes.') | |||
There was a problem hiding this comment.
That's an indirect test - I would just make this a method of the test case, and do self.assertIsInstance(x, bytes).
fakeredis.py
Outdated
| @@ -1619,16 +1621,17 @@ def zrange(self, name, start, end, desc=False, withscores=False): | |||
| if not withscores: | |||
| return items | |||
| else: | |||
| return [(k, all_items[k]) for k in items] | |||
| return [(k, score_cast_func(bytes(all_items[k]))) | |||
There was a problem hiding this comment.
You can't convert a float to bytes like this in Python 3. Use the to_bytes function instead, which goes via repr.
fferrara
force-pushed
the
zrangebyscore-new-argument
branch
from
f493361
to
777e25e
Compare
|
@bmerry I tried to implement what you intended. Is that right? |
|
There's one very minor tweak I'd like to make in the test, but I'll just merge and make the tweak myself. Thanks for the work on this - it's been a missing feature for quite a while. |
|
Thank you for the support! How long do you think until we have a new tag/version? |
|
I should have time to cut a new version some time in the next week. |
When decode_responses is true, the score_cast_func is passed text, not bytes. To handle this, the score_cast_func handling code was unified in one helper function which deals with the different cases. It also has a cut-through path for the default score_cast_func to avoid a lot of unnecessarily float -> bytes -> text -> float casting. The unit test is made stricted to check the type exactly.
|
@bmerry can we have a release for that? I'd like to clean up the weird hacks to work around this error |
|
Done. |
Fixes #193 #44