feat(command): Add subcommand abstraction #3387
feat(command): Add subcommand abstraction #3387jihuayu wants to merge 16 commits intoapache:unstablefrom
Conversation
… duplicate registration tests
jihuayu
changed the title
|
| using SubcommandMap = std::map<std::string, const CommandAttributes *, std::less<>>; | ||
|
|
||
| std::optional<std::string> ResolveSubcommandByArgIndex(const std::vector<std::string> &args, size_t index); | ||
| SubcommandResolver MakeArgIndexSubcommandResolver(size_t index); |
There was a problem hiding this comment.
These seem overly abstract to me.
| REDIS_REGISTER_SUBCOMMANDS( | ||
| Server, "namespace", MakeArgIndexSubcommandResolver(1), DefaultSubcommandFallback(), | ||
| MakeSubCmdAttr<CommandNamespaceGet>("namespace", "get", 3, "read-only admin skip-monitor", NO_KEY), | ||
| MakeSubCmdAttr<CommandNamespaceSet>("namespace", "set", 4, "read-only admin skip-monitor", NO_KEY), | ||
| MakeSubCmdAttr<CommandNamespaceAdd>("namespace", "add", 4, "read-only admin skip-monitor", NO_KEY), | ||
| MakeSubCmdAttr<CommandNamespaceDel>("namespace", "del", 3, "read-only admin skip-monitor", NO_KEY), | ||
| MakeSubCmdAttr<CommandNamespaceCurrent>("namespace", "current", 2, "read-only skip-monitor", NO_KEY)) |
There was a problem hiding this comment.
I think we should just use REDIS_REGISTER_COMMANDS instead a new REDIS_REGISTER_SUBCOMMANDS.
|
@PragmaTwice Thank you very much for your feedback; I consider it crucial. As you noted, the code was generated with LLM assistance, but every design choice was the result of my own deliberation and refinement. I reviewed every line myself. I did have some reservations, feeling that the code was becoming a bit too complex and difficult to review, but since I felt the changes were justified, I wanted to submit them to see what the community thought. Your feedback made me realize that LLMs naturally tend to over-expand abstractions in an attempt to build something "all-encompassing." I've learned that I should minimize the scope of abstraction as much as possible to make the code more review-friendly. The use of an LLM isn't the mistake—excessive complexity is. Thank you again for your review. I will be making adjustments accordingly. PS: I believe that "too complex and difficult to review" is, in itself, excellent feedback. |
2 participants
I have introduced the subcommand framework while making every effort to avoid modifying the existing command registration and execution logic. I plan to wait until the subcommand functionality is stable and fully migrated before performing a unified refactoring.
Currently, I have modified the namespace command as a reference example.
Some important
New concept Subcommand Family: Organizes a root command and its multiple subcommands into a single command group. A command family consists of three parts: the root command name, the subcommand parser, and the subcommand table.
New concept ResolvedCommand (Subcommand Resolution Result): Instead of simply identifying the "first token input by the user," this explicitly distinguishes between the "root command" and the "actual command to be executed." For example, NAMESPACE ADD would be resolved as root = namespace, with the final command being namespace|add.
New concept Canonical Name: Introduces a unified internal naming convention root|sub for subcommands (e.g., namespace|add). This name is used for internal capabilities such as COMMAND INFO, profiling, and statistics, removing the dependency on the raw input text.
Registration-time Conflict Prevention: Adds startup-time validation for scenarios such as duplicate command family registration, duplicate subcommand registration, or parent-child command mismatches to prevent silent overrides.