-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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 ZRANDMEMBER #2076
Support ZRANDMEMBER #2076
Conversation
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 started making suggested changes to plural naming (to match existing methods) for the array cases but after many comments I decided pushing the changes up would be decidedly less annoying :)
This looks great - I tweaked docs and methods, only thing missing I think is testing on the singular methods.
Edit: we should also test the missing key cases (need not be different test methods) to ensure that behaves as expected - thought of this looking at another PR.
Thank you for the help with the docs! |
This also gives clearer errors about what wasn't contained.
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.
Looking good, 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.
1 comment about the API - looks like it's missing 1 pair of methods.
/// <param name="flags">The flags to use for this operation.</param> | ||
/// <returns>The randomly selected element, or <see cref="RedisValue.Null"/> when <paramref name="key"/> does not exist.</returns> | ||
/// <remarks>https://redis.io/commands/zrandmember</remarks> | ||
RedisValue SortedSetRandomMember(RedisKey key, CommandFlags flags = CommandFlags.None); |
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.
Noodling on this, probably want to have a SortedSetRandomMemberWithScore
method? Where you just pass in COUNT
1 and the WITHSCORES
argument to yield a single SortedSetResult.
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 was thinking: it's rare enough I'm okay leaving it off - if there was ever a complaint, we could add it later.
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.
@slorello89 If good with the above, I'll go ahead and get this in - thoughts?
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.
👍
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.
LGTM
/// <param name="flags">The flags to use for this operation.</param> | ||
/// <returns>The randomly selected element, or <see cref="RedisValue.Null"/> when <paramref name="key"/> does not exist.</returns> | ||
/// <remarks>https://redis.io/commands/zrandmember</remarks> | ||
RedisValue SortedSetRandomMember(RedisKey key, CommandFlags flags = CommandFlags.None); |
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.
👍
https://redis-stack.io/commands/zrandmember/
#2055