Add Very Big Enum with all commands to protobuf #220
Conversation
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
Yury-Fridlyand
requested review from
aaron-congo,
jonathanl-bq,
alex-arzola-imp,
acarbonetto and
SanHalacogluImproving
| AclWhoami = 1112; | ||
| BgRewriteAof = 1113; | ||
| BgSave = 1114; | ||
| Command_ = 1115; // TODO renaming to `Command` causes protobuf error |
There was a problem hiding this comment.
If I rename this to Command, protobuf returns
protobuf/redis_request.proto:268:5: "Command" is already defined in "redis_request".
protobuf/redis_request.proto:268:5: Note that enum values use C++ scoping rules, meaning that enum values are siblings of their type, not children of it. Therefore, "Command" must be unique within "redis_request", not just within "RequestType".
It conflicts with Command on line 590. Which one to rename?
There was a problem hiding this comment.
It would probably impact less code to rename the Command here instead of the one on line 590. However, I think the "right" way to do this is to rename the one at line 590 in a separate PR.
| PSetEx = 1518; // deprecated in 2.6.12 | ||
| SetString = 1519; // Set | ||
| SetEx = 1520; // deprecated in 2.6.12 | ||
| SetNx = 1521; // deprecated in 2.6.12 |
There was a problem hiding this comment.
keep it there to avoid blanks
| SetRange = 107; | ||
| ZRemRangeByLex = 108; | ||
| ZLexCount = 109; | ||
| GetString = 1504; // Get |
There was a problem hiding this comment.
Will rename to Get and Set if no objections
There was a problem hiding this comment.
I like the idea. It keeps things more consistent IMO.
| LatencyHistory = 1135, | ||
| LatencyLatest = 1136, | ||
| LatencyReset = 1137, | ||
| Lolwut = 1138, |
There was a problem hiding this comment.
I can't believe this is actually real... https://redis.io/docs/latest/commands/lolwut/
There was a problem hiding this comment.
I tested it with custom command and looking forward adding implementation of that!
| Echo = 320; | ||
| Hello = 321; | ||
| Ping = 322; | ||
| Quit = 323; // deprecated in 7.2.0 |
There was a problem hiding this comment.
Do we really need to add the deprecated variants? I guess it doesn't hurt, but I wonder what the AWS side will want.
| SetRange = 107; | ||
| ZRemRangeByLex = 108; | ||
| ZLexCount = 109; | ||
| GetString = 1504; // Get |
There was a problem hiding this comment.
I like the idea. It keeps things more consistent IMO.
| AclWhoami = 1112; | ||
| BgRewriteAof = 1113; | ||
| BgSave = 1114; | ||
| Command_ = 1115; // TODO renaming to `Command` causes protobuf error |
There was a problem hiding this comment.
It would probably impact less code to rename the Command here instead of the one on line 590. However, I think the "right" way to do this is to rename the one at line 590 in a separate PR.
| Append = 1501, | ||
| Decr = 1502, | ||
| DecrBy = 1503, | ||
| /// Type of a get string request. |
There was a problem hiding this comment.
This comment feels kind of random and out of place.
| ProtobufRequestType::HashGet => RequestType::HashGet, | ||
| ProtobufRequestType::HashDel => RequestType::HashDel, | ||
| ProtobufRequestType::HashExists => RequestType::HashExists, | ||
| ProtobufRequestType::HSet => RequestType::HSet, |
There was a problem hiding this comment.
I like these changes but we might need to split them into a separate PR due to how large this PR already is. Not a strong opinion though, and not a blocker.
| RequestType::ZMScore => Some(cmd("ZMSCORE")), | ||
| RequestType::ZDiff => Some(cmd("ZDIFF")), | ||
| RequestType::ZDiffStore => Some(cmd("ZDIFFSTORE")), | ||
| RequestType::SetRange => Some(cmd("SETRANGE")), | ||
| RequestType::ZRemRangeByLex => Some(cmd("ZREMRANGEBYLEX")), | ||
| RequestType::ZLexCount => Some(cmd("ZLEXCOUNT")), | ||
| _ => todo!(), |
There was a problem hiding this comment.
Are there actually more commands we're missing here? Or is this just a catch-all case for future added commands?
| @@ -130,7 +130,7 @@ public static Expiry UnixMilliseconds(Long unixMilliseconds) { | |||
| /** Types of value expiration configuration. */ | |||
| @RequiredArgsConstructor | |||
| protected enum ExpiryType { | |||
| KEEP_EXISTING("KEEPTTL"), | |||
| KEEP_EXISTING("KEEPTtl"), | |||
There was a problem hiding this comment.
This looks a bit weird and inconsistent with the other entries here now.
there are 4 changes:
HvalstoHValsHashDeltoHDelAll renames (except
HashSet) were done withCtrl+Shift+Fmagic