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

Go: BZPOPMAX and ZMPOP #3017

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

MikeMwita
Copy link
Collaborator

@MikeMwita MikeMwita commented Jan 27, 2025

Issue link

This Pull Request is linked to issue: #2971

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one issue.
  • Commit message has a detailed description of what changed and why.
  • Tests are added or updated.
  • CHANGELOG.md and documentation files are updated.
  • Destination branch is correct - main or release
  • Create merge commit if merging release branch into main, squash otherwise.

@MikeMwita MikeMwita requested a review from a team as a code owner January 27, 2025 20:51
@Yury-Fridlyand Yury-Fridlyand added the go golang wrapper label Jan 27, 2025
go/api/base_client.go Outdated Show resolved Hide resolved
go/api/base_client.go Outdated Show resolved Hide resolved
go/api/command_options.go Outdated Show resolved Hide resolved
go/api/command_options.go Outdated Show resolved Hide resolved
go/api/response_handlers.go Outdated Show resolved Hide resolved
go/api/response_types.go Outdated Show resolved Hide resolved
@MikeMwita MikeMwita force-pushed the add-zmpop-and-bzpopmax-imple branch from 821c1a1 to e354de2 Compare February 8, 2025 13:11
@MikeMwita MikeMwita changed the title WIP: Go: BZPOPMAX and ZMPOP Go: BZPOPMAX and ZMPOP Feb 8, 2025
@MikeMwita MikeMwita force-pushed the add-zmpop-and-bzpopmax-imple branch 4 times, most recently from 7ab8c90 to cf4fcf2 Compare February 8, 2025 19:02
@MikeMwita MikeMwita requested review from tjzhang-BQ and edlng February 9, 2025 13:52
@MikeMwita MikeMwita force-pushed the add-zmpop-and-bzpopmax-imple branch 6 times, most recently from d3a7fcb to ae55983 Compare February 13, 2025 19:34
Signed-off-by: MikeMwita <[email protected]>
@MikeMwita MikeMwita force-pushed the add-zmpop-and-bzpopmax-imple branch from ae55983 to 101427f Compare February 13, 2025 19:47
@@ -45,6 +45,10 @@ type SortedSetCommands interface {
options *options.ZMPopOptions,
) (Result[KeyWithArrayOfMembersAndScores], error)

BZPopMax(keys []string, timeoutSecs float64) (Result[KeyWithMemberAndScore], error)

ZMPop(keys []string, scoreFilter ScoreFilter) (Result[KeyWithArrayOfMembersAndScores], error)
Copy link
Collaborator

Choose a reason for hiding this comment

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

As mentioned in this comment: #3017 (comment), the count parameter needs to be added in a separate definition as it is optional.

@@ -201,6 +201,11 @@ func toCStrings(args []string) ([]C.uintptr_t, []C.ulong) {
//
// `"OK"` response on success.
//
// For example:
Copy link
Contributor

@edlng edlng Feb 13, 2025

Choose a reason for hiding this comment

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

Remove all these added example comments in base_client.go. We now add examples in the api/*_test.go files.

Comment on lines +6868 to +6872
// Example:
//
// zaddResult, err := client.ZAdd("mySortedSet", map[string]float64{"a": 1.0, "b": 2.0, "c": 3.0})
// res, err := client.BZPopMax([]string{"mySortedSet"}, 1.0)
// fmt.Println(res.Value()) // Output: {Key: "mySortedSet", Member: "c", Score: 3.0}
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this to api/sorted_set_commands_test.go instead and create examples for ExampleGlideClient and ExampleGlideClusterClient. Feel free to look at the DEVELOPER.md for more details about the new way to write examples

Comment on lines +6908 to +6911
// Example:
//
// res, err := client.ZMPop([]string{"mySortedSet"}, api.MAX, 2)
// fmt.Println(res.Value()) // Output: {Key: "mySortedSet", MembersAndScores: [{Member: "c", Score: 3.0}, {Member: "b", Score:
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this to api/sorted_set_commands_test.go instead and create examples for ExampleGlideClient and ExampleGlideClusterClient.

Copy link
Collaborator

@Yury-Fridlyand Yury-Fridlyand left a comment

Choose a reason for hiding this comment

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

  1. Need to move examples to the corresponding file(s).
  2. Need to add a changelog entry as of now.

Please wait for backporting all changes from release-1.3 branch. You can merge/rebase you branch after that and implement required fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go golang wrapper
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants