feat: add shell completion support#227
feat: add shell completion support#227david-waltermire merged 3 commits intometaschema-framework:developfrom
Conversation
Register SPI-discovered commands including shell-completion from cli-processor. This enables users to generate bash and zsh completion scripts for oscal-cli. Usage: oscal-cli shell-completion bash > completion.bash oscal-cli shell-completion zsh > completion.zsh Added comprehensive tests for: - Bash and zsh completion script generation - File output with --to option - Invalid shell type handling - Missing argument handling Closes metaschema-framework#85
📝 WalkthroughWalkthroughRegisters SPI-discovered commands into the CLI processor at runtime and adds tests exercising the shell-completion command (help, bash/zsh generation, file output, invalid args, missing args). Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI_Main as CLI.main
participant Processor
participant CommandService
participant SPI_Command as ShellCompletionCommand
User->>CLI_Main: start with args
CLI_Main->>Processor: register built-in commands
CLI_Main->>CommandService: getInstance().getCommands()
CommandService-->>CLI_Main: list of discovered commands
CLI_Main->>Processor: addCommandHandler(discovered commands)
CLI_Main->>Processor: process(args)
Processor->>SPI_Command: invoke matching command
SPI_Command-->>User: output (help/script/file/error)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/test/java/dev/metaschema/oscal/tools/cli/core/ShellCompletionTest.java (1)
78-99: Consider using try-finally or @AfterEach for reliable cleanup.The test properly validates file output, but the cleanup at line 98 won't execute if an assertion fails. While this is acceptable for CI environments (target/ is typically cleaned), consider moving cleanup to
@AfterEachor wrapping in try-finally for better test isolation.♻️ Optional improvement for cleanup reliability
@ParameterizedTest @ValueSource(strings = { "bash", "zsh" }) void testShellCompletionToFile(String shell) throws IOException { Path outputPath = Path.of("target/completion." + shell); String[] args = { "shell-completion", shell, "--to", outputPath.toString() }; - ExitStatus status = CLI.runCli(args); - - assertAll( - () -> assertEquals(ExitCode.OK, status.getExitCode(), - "shell-completion " + shell + " --to file should return OK"), - () -> assertTrue(Files.exists(outputPath), - "Output file should exist"), - () -> { - String content = Files.readString(outputPath, StandardCharsets.UTF_8); - assertTrue(content.contains("oscal-cli"), - "Completion script should reference oscal-cli"); - }); - - // Clean up - Files.deleteIfExists(outputPath); + try { + ExitStatus status = CLI.runCli(args); + + assertAll( + () -> assertEquals(ExitCode.OK, status.getExitCode(), + "shell-completion " + shell + " --to file should return OK"), + () -> assertTrue(Files.exists(outputPath), + "Output file should exist"), + () -> { + String content = Files.readString(outputPath, StandardCharsets.UTF_8); + assertTrue(content.contains("oscal-cli"), + "Completion script should reference oscal-cli"); + }); + } finally { + // Clean up + Files.deleteIfExists(outputPath); + } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/dev/metaschema/oscal/tools/cli/core/CLI.javasrc/test/java/dev/metaschema/oscal/tools/cli/core/ShellCompletionTest.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Website
- GitHub Check: Code
🔇 Additional comments (8)
src/main/java/dev/metaschema/oscal/tools/cli/core/CLI.java (2)
10-10: LGTM!The import is necessary for the SPI-discovered command registration.
84-86: SPI command registration is safe as-is - tests confirm functionality.The tests in CLITest.java invoke
CLI.runCli()multiple times successfully without error handling aroundCommandService.getInstance().getCommands(). The code follows the established pattern used elsewhere in the codebase (e.g., lines 72-83) where command registration has no defensive wrapping. SPI discovery is working as intended in the test suite.The thread-safety concern relates to the external
CommandServiceimplementation in metaschema-framework, not this code. The CLI runs sequentially and callsgetInstance()once during initialization, which is not a multi-threaded scenario requiring additional synchronization at this point.src/test/java/dev/metaschema/oscal/tools/cli/core/ShellCompletionTest.java (6)
36-43: LGTM!The test appropriately verifies that the help flag is recognized and returns a success exit code.
48-58: LGTM!The test properly validates successful bash completion generation with both exit code and exception checks.
63-73: LGTM!The test validates zsh completion generation with appropriate assertions.
104-112: LGTM!The test appropriately validates error handling for unsupported shell types.
117-125: LGTM!The test properly validates the requirement for the shell argument.
1-31: LGTM!The test class is well-structured with clear documentation, appropriate imports, and comprehensive test coverage addressing issue #85.
Address CodeRabbit review feedback - wrap file output test in try-finally to ensure cleanup runs even if assertions fail.
|
@coderabbitai Addressed the nitpick in commit faadf0d - wrapped file output test in try-finally to ensure cleanup runs even if assertions fail. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2609 @david-waltermire: Thank you for addressing the feedback! I've verified commit faadf0d and the change looks great. The try-finally block ensures that the cleanup code in |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/test/java/dev/metaschema/oscal/tools/cli/core/ShellCompletionTest.java (2)
81-81: Consider Maven build directory dependency.The hardcoded
"target/"path assumes Maven project structure. While this is standard for Maven projects and should work fine, be aware this might fail in non-standard build configurations or if the target directory doesn't exist before tests run.
93-95: Consider more thorough content validation.The test only verifies that "oscal-cli" appears in the generated script. While this provides basic smoke-testing, you could optionally validate:
- The script contains expected shell syntax patterns
- Specific command names appear in completions
- The script structure matches bash/zsh completion conventions
However, the current minimal validation may be sufficient depending on your testing strategy.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/java/dev/metaschema/oscal/tools/cli/core/ShellCompletionTest.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Website
🔇 Additional comments (6)
src/test/java/dev/metaschema/oscal/tools/cli/core/ShellCompletionTest.java (6)
25-31: Good documentation linking to issue #85.The class-level documentation clearly references the related issue, which helps with traceability.
33-43: Well-structured help test.The test appropriately verifies that help output returns a successful exit code with clear assertion messages.
45-73: Good use of assertAll for grouped assertions.Both tests appropriately use
assertAllto verify multiple conditions (exit code and absence of exceptions), which provides better test failure reporting.
75-101: Excellent cleanup pattern with try-finally.The try-finally block ensures file cleanup occurs even if assertions fail, which prevents test pollution. This is a best practice for tests that create filesystem resources.
103-114: Good negative test case for invalid input.Testing the error path with an unsupported shell type (fish) ensures the command fails gracefully with an appropriate exit code.
116-127: Appropriate edge case for missing arguments.Testing the missing required argument scenario ensures proper validation and user feedback.
david-waltermire
merged commit 08ea1fe
into
metaschema-framework:develop
1 participant
Summary
shell-completionfrom cli-processorUsage
Changes
CLI.javato register SPI-discovered commands viaCommandServiceShellCompletionTest.javawith comprehensive testsDependencies
CommandService)Test plan
--tooption works correctlyCloses #85
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.