Skip to content
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

Merged
merged 9 commits into from
Apr 12, 2022
Merged

Conversation

Avital-Fine
Copy link
Contributor

@Avital-Fine Avital-Fine commented Apr 10, 2022

Copy link
Collaborator

@NickCraver NickCraver left a 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.

@Avital-Fine
Copy link
Contributor Author

Avital-Fine commented Apr 11, 2022

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!
I added tests for missing key cases but didn't understand what did you mean by singular methods 😅

This also gives clearer errors about what wasn't contained.
Copy link
Collaborator

@NickCraver NickCraver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, thanks!!

Copy link
Collaborator

@slorello89 slorello89 left a 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);
Copy link
Collaborator

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.

Copy link
Collaborator

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.

Copy link
Collaborator

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Collaborator

@slorello89 slorello89 left a 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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@NickCraver NickCraver merged commit cd33beb into StackExchange:main Apr 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants