refactor(cli): self-register commands via ServiceLoader#171
Merged
Conversation
ArgusCli.java held a 67-line manual registration list β every command class had to be imported and added to a LinkedHashMap by hand, and the ordering of that list happened to drive help output. Adding a command required touching one place outside the command's own file. Move discovery to a META-INF/services/io.argus.cli.command.Command manifest. ArgusCli loads commands with ServiceLoader; help output sorts alphabetically within each CommandGroup so behaviour no longer depends on registration order. TuiCommand stays explicitly registered after the loader pass because it takes the populated commands map as a constructor argument and so has no public no-arg constructor that ServiceLoader requires. Scope note: this PR does not unify CLI Command with the server-side DiagnosticCommand SPI. Those interfaces overlap on metadata (name/group/description) but diverge on the execute() shape β CLI commands carry ANSI/TUI/--format=json/CliConfig concerns the server context object does not have. Unification would invade ~80 files for a mostly-cosmetic win, while the registration friction (one central list) is what actually hurt. Helps output went from 67 manual lines to 66 declarative lines plus alphabetical-within-group ordering. ArgusCli.java: -149 lines. Signed-off-by: rlaope <piyrw9754@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ArgusCli.javaheld a 67-line manual registration list. Every command had to be imported and added to aLinkedHashMapby hand, and the ordering of that list happened to drive help output.This PR moves discovery to a
META-INF/services/io.argus.cli.command.Commandmanifest.ArgusCliloads commands viaServiceLoaderand sorts within eachCommandGroupalphabetically for the help screen β so behaviour no longer depends on registration order.TuiCommandstays explicitly registered after the loader pass because it takes the populatedcommandsmap as a constructor argument and so cannot satisfyServiceLoader's no-arg-constructor requirement.ArgusCli.java: β149 lines (66 manualregistercalls + 67 imports gone)META-INF/services/io.argus.cli.command.Command: +66 lines (one FQN per command)Why this and not full SPI unification
The earlier architectural sketch suggested merging CLI
Commandwith the server-sideDiagnosticCommandSPI. That overlap exists on metadata (name/group/description) but theexecute(...)shapes diverge β CLI commands carry ANSI / TUI /--format=json/CliConfigconcerns thatCommandContextdoesn't model, and only ~29 of the 67 CLI commands have a server sibling. A full unification would touch ~80 files for a mostly-cosmetic win. The actual friction was the central manual registration list, which this PR eliminates.Behaviour changes
CommandGroup. Previous order was a hand-curated list that drifted as commands were added at the end. The group order itself (theCommandGroupenum's declaration order) is unchanged.ArgusCli.javaβ drop the class and add one line to the META-INF file.Test plan
./gradlew build -x integrationTestpassesargus --helpshows "67 commands" line