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.

Sign up for GitHub

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 PING command #2264

Merged
merged 1 commit into from
Oct 9, 2024
Merged

Go: Implementing PING command #2264

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 29 additions & 1 deletion go/api/base_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ type BaseClient interface {
StringCommands
HashCommands
ListCommands

ConnectionManagementCommands
// Close terminates the client by closing all associated resources.
Close()
}
Expand Down Expand Up @@ -512,3 +512,31 @@ func (client *baseClient) RPush(key string, elements []string) (Result[int64], e

return handleLongResponse(result)
}

func (client *baseClient) Ping() (string, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should return Result[string] from all the methods to keep it consistent and pass the nil string info if an error has occurred. Can you please address this in another PR?

result, err := client.executeCommand(C.Ping, []string{})
if err != nil {
return "", err
}

response, err := handleStringResponse(result)
if err != nil {
return "", err
}
return response.Value(), nil
}

func (client *baseClient) PingWithMessage(message string) (string, error) {
args := []string{message}

result, err := client.executeCommand(C.Ping, args)
if err != nil {
return "", err
}

response, err := handleStringResponse(result)
if err != nil {
return "", err
}
return response.Value(), nil
}
34 changes: 34 additions & 0 deletions go/api/commands.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -692,3 +692,37 @@ type HashCommands interface {
// [valkey.io]: https://valkey.io/commands/hstrlen/
HStrLen(key string, field string) (Result[int64], error)
}

// ConnectionManagementCommands defines an interface for connection management-related commands.
//
// See [valkey.io] for details.
type ConnectionManagementCommands interface {
// Pings the server.
//
// If no argument is provided, returns "PONG". If a message is provided, returns the message.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's have two separate implementations of Ping in this case. Ping() with no arguments and PingMessage(message key). I think that would be cleaner.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ping()
ping(str)

and for cluster client

ping()
ping(str)
ping(route)
ping(str, route)

//
// Return value:
// If no argument is provided, returns "PONG".
// If an argument is provided, returns the argument.
//
// For example:
// result, err := client.Ping("Hello")
Copy link
Collaborator

Choose a reason for hiding this comment

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

The Ping method doesn't receive an argument here.

//
// [valkey.io]: https://valkey.io/commands/ping/
Ping() (string, error)

// Pings the server with a custom message.
//
// If a message is provided, returns the message.
// If no argument is provided, returns "PONG".
//
// Return value:
// If no argument is provided, returns "PONG".
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to not provide an argument in this method?

// If an argument is provided, returns the argument.
//
// For example:
// result, err := client.PingWithMessage("Hello")
//
// [valkey.io]: https://valkey.io/commands/ping/
PingWithMessage(message string) (string, error)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are golang docstrings propagated to all API in such case? If no or you'r not sure - please duplicate docs for PingWithMessage.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can check it with a doc generator.

}
17 changes: 17 additions & 0 deletions go/integTest/shared_commands_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -588,6 +588,23 @@ func (suite *GlideTestSuite) TestGetDel_EmptyKey() {
})
}

func (suite *GlideTestSuite) TestPing_NoArgument() {
suite.runWithDefaultClients(func(client api.BaseClient) {
result, err := client.Ping()
assert.Nil(suite.T(), err)
assert.Equal(suite.T(), "PONG", result)
})
}

func (suite *GlideTestSuite) TestPing_WithArgument() {
suite.runWithDefaultClients(func(client api.BaseClient) {
// Passing "Hello" as the message
result, err := client.PingWithMessage("Hello")
assert.Nil(suite.T(), err)
assert.Equal(suite.T(), "Hello", result)
})
}

func (suite *GlideTestSuite) TestHSet_WithExistingKey() {
suite.runWithDefaultClients(func(client api.BaseClient) {
fields := map[string]string{"field1": "value1", "field2": "value2"}
Expand Down
Loading