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 the ZMSCORE command. #2082

Merged
merged 8 commits into from
Apr 14, 2022
Merged

Support the ZMSCORE command. #2082

merged 8 commits into from
Apr 14, 2022

Conversation

ttingen
Copy link
Contributor

@ttingen ttingen commented Apr 12, 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 pushed up doc tweaks for ease there, just a few requests on the processor and tests (happy to push to branch if easier and you're slammed). Thanks for this!!

src/StackExchange.Redis/ResultProcessor.cs Outdated Show resolved Hide resolved
tests/StackExchange.Redis.Tests/SortedSets.cs Outdated Show resolved Hide resolved
@ttingen
Copy link
Contributor Author

ttingen commented Apr 12, 2022

@NickCraver all done with the requested changes. Let me know if there is anything else.

@NickCraver
Copy link
Collaborator

@ttingen Made a few quick changes to normalization and naming (I see why keys in tests were confusing where above we did need 2 unique keys - sorry about that!) - will try to do a top-down review in a bit!

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.

@ttingen I think current is good - thank you! I'd appreciate @slorello89 eyes here when time allows.

@NickCraver
Copy link
Collaborator

@slorello89 @Avital-Fine Can one of y'all provide secondary eyes here please?

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.

Somehow missed the request @NickCraver sorry about that! LGTM 👍

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