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

DOC-4494 SADD and SMEMBERS command examples #3242

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

andy-stark-redis
Copy link
Contributor

@andy-stark-redis andy-stark-redis commented Jan 22, 2025

DOC-4494 and DOC-4436.

Go examples for the SADD and SMEMBERS command pages.

@ndyakov ndyakov self-requested a review February 7, 2025 08:43
ndyakov
ndyakov previously approved these changes Feb 7, 2025
// REMOVE_END

// STEP_START smembers
sMembersResult1, err := rdb.SAdd(ctx, "myset", "Hello", "World").Result()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a result of SAdd


fmt.Println(sAddResult3) // >>> 0

sAddResult4, err := rdb.SMembers(ctx, "myset").Result()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a result of SMembers

Copy link
Collaborator

@ndyakov ndyakov left a comment

Choose a reason for hiding this comment

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

Hey @andy-stark-redis , should we rename the variables based on the comments I left?

@andy-stark-redis
Copy link
Contributor Author

Hey @andy-stark-redis , should we rename the variables based on the comments I left?

@ndyakov The reason for the names is that all the variables with names like sMembersResultxx are in the examples for the SMEMBERS command page and likewise for sAddResultxx, etc. If you think it's confusing then we could use a different naming convention, but we probably don't want to name the result variables after the commands that return them, in general.

@ndyakov
Copy link
Collaborator

ndyakov commented Feb 10, 2025

@andy-stark-redis let's try to come up with another naming convention. I do think this can be confusing to someone reading the code. Maybe something like mysetMembers for the result of the SMembers(...,"myset") ? We can brainstorm that on slack if you would like?

@ndyakov
Copy link
Collaborator

ndyakov commented Feb 21, 2025

@andy-stark-redis are we keeping those variable names or we are waiting for an update on this PR?

@andy-stark-redis
Copy link
Contributor Author

@andy-stark-redis are we keeping those variable names or we are waiting for an update on this PR?

@ndyakov I've added a new commit to change the names already. I think they are now in line with what you suggested but I'll change them if you still think they are unhelpful.

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.

2 participants