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

Add ZMSCORE command support #118

Merged
merged 8 commits into from
Mar 28, 2024
Merged

Add ZMSCORE command support #118

merged 8 commits into from
Mar 28, 2024

Conversation

ProTip
Copy link
Contributor

@ProTip ProTip commented Mar 24, 2024

I have tried my hand at adding the ZMSCORE command using the(very helpful) direction in the issue and surrounding code.

The score/scores fetching implementations could likely be merged.

resolves #77

@ProTip
Copy link
Contributor Author

ProTip commented Mar 24, 2024

@microsoft-github-policy-service agree

@badrishc
Copy link
Contributor

Make sure you use the error handling logic introduced in this recent PR: #103

@ProTip
Copy link
Contributor Author

ProTip commented Mar 25, 2024

Make sure you use the error handling logic introduced in this recent PR: #103

I believe I am using the newly introduced command, however I have improved the count check and removed the arithmetic to be inline with the changes in the referenced PR.

@badrishc badrishc requested a review from TalZaccai March 26, 2024 01:11
Copy link
Contributor

@TalZaccai TalZaccai left a comment

Choose a reason for hiding this comment

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

Great work, thank you for your contribution to Garnet!
Added a few comments here :)

test/Garnet.test/RespSortedSetTests.cs Show resolved Hide resolved
libs/server/API/IGarnetApi.cs Outdated Show resolved Hide resolved
libs/server/Objects/SortedSet/SortedSetObjectImpl.cs Outdated Show resolved Hide resolved
libs/server/Resp/RespCommand.cs Outdated Show resolved Hide resolved
libs/server/Resp/Objects/SortedSetCommands.cs Outdated Show resolved Hide resolved
libs/server/Resp/Objects/SortedSetCommands.cs Show resolved Hide resolved
test/Garnet.test/RespSortedSetTests.cs Outdated Show resolved Hide resolved
@TalZaccai TalZaccai self-assigned this Mar 26, 2024
@ProTip ProTip requested a review from TalZaccai March 26, 2024 09:40
@ProTip
Copy link
Contributor Author

ProTip commented Mar 26, 2024

Cheers, that was a very good review😅 I believe I have addressed all the issues less some potential clarification in the unresolved review convos.

@vazois vazois added the API label Mar 26, 2024
Copy link
Contributor

@TalZaccai TalZaccai left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks again for your contribution! :)

@TalZaccai TalZaccai merged commit 618880c into microsoft:main Mar 28, 2024
13 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators May 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

API Coverage - Implement ZMSCORE
4 participants