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: Implementing Copy command #2480

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MikeMwita
Copy link
Collaborator

No description provided.

@MikeMwita MikeMwita requested a review from a team as a code owner October 20, 2024 15:10
@Yury-Fridlyand Yury-Fridlyand added the go golang wrapper label Oct 21, 2024
@@ -30,4 +30,30 @@ type GenericBaseCommands interface {
//
// [valkey.io]: https://valkey.io/commands/del/
Del(keys []string) (Result[int64], error)

// Copy copies the value stored at the source key to the destination key.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// Copy copies the value stored at the source key to the destination key.
// Copies the value stored at the source key to the destination key.

Comment on lines +38 to +39
// Note:
// - When in cluster mode, the command may route to multiple nodes depending on the location of the source and destination keys.
Copy link
Collaborator

Choose a reason for hiding this comment

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

when in cluster, both keys should be managed by the same node.

Please use another notice.

// Parameters:
// sourceKey - The key from which to copy the value.
// destinationKey - The key to copy the value to.
// destinationDB - An optional integer representing the database index for the destination key. If nil, the current database is used.
Copy link
Collaborator

Choose a reason for hiding this comment

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

How we do in docs - nil or nil?

Suggested change
// destinationDB - An optional integer representing the database index for the destination key. If nil, the current database is used.
// destinationDB - An optional integer representing the database index for the destination key. If `nil`, the current database is used.

// replace - A boolean flag indicating whether to replace the destination key if it exists.
//
// Return value:
// Returns 1 if the copy was successful, or 0 if the destination key already exists without the `REPLACE` option.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Returns true/false
Please refer to (or even copy) docs from other clients.

Comment on lines +1987 to +1988
key1 := "dolly_" + uuid.New().String()
key2 := "clone_" + uuid.New().String()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will fail on a cluster

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.

2 participants