fix: ping return bulk string #933
Conversation
| require.NoError(t, c.Write("*1\r\n$4\r\nPING\r\n")) | ||
| r, err := c.ReadLine() | ||
| require.NoError(t, err) | ||
| require.Contains(t, r, "+PONG") |
There was a problem hiding this comment.
Should we write a new test case for the redis protocol validation about PING?
It seems that semantic verification and protocol verification have been put into one test case, which I think should be split into two cases.
There was a problem hiding this comment.
Yeah. Maybe it is not necessary to verify the protocol about PING. Some things are apparent. It makes the test too cumbersome.
…ocks into ping-return-bulk-string
| @@ -5833,7 +5839,7 @@ class CommandXTrim : public Commander { | |||
|
|
|||
| CommandAttributes redisCommandTable[] = { | |||
| ADD_CMD("auth", 2, "read-only ok-loading", 0, 0, 0, CommandAuth), | |||
| ADD_CMD("ping", 1, "read-only", 0, 0, 0, CommandPing), | |||
| ADD_CMD("ping", -1, "read-only", 0, 0, 0, CommandPing), | |||
There was a problem hiding this comment.
May I ask the reason to make this change? Is there some reference from the Redis document?
The overall change looks good to me. Merging...
There was a problem hiding this comment.
This digit indicates the number of parameters allowed(including command name), "1 " means there is precisely one, and "-1 "means at least one. Do you want this source code and the comment?
This closes #926.