Conversation
Reviewer's GuideIntroduces a clap-based CLI with file, directory, and jobs options; build, clean, and graph subcommands defaulting to build; comprehensive unit and behavioural (Cucumber) tests; documentation updates; and required dependency additions and test harness configuration. Class diagram for the new CLI structureclassDiagram
class Cli {
+PathBuf file
+Option<PathBuf> directory
+Option<usize> jobs
+Option<Commands> command
}
class Commands {
}
Cli --> Commands
class Build {
+Vec<String> targets
}
class Clean {
}
class Graph {
}
Commands <|-- Build
Commands <|-- Clean
Commands <|-- Graph
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 50 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
Summary by CodeRabbitNew Features
Documentation
Tests
Summary by CodeRabbit
WalkthroughIntroduce a new command-line interface (CLI) for the Netsuke application using the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI (clap)
participant Main
participant CommandHandler
User->>CLI (clap): Provide command-line arguments
CLI (clap)->>Main: Parse into Cli struct
Main->>CommandHandler: Match on Cli.command
alt Build command or no command
CommandHandler->>Main: Print build targets
else Clean command
CommandHandler->>Main: Print clean request
else Graph command
CommandHandler->>Main: Print graph request
end
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and found some issues that need to be addressed.
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `tests/cli_tests.rs:6` </location>
<code_context>
+use rstest::rstest;
+use std::path::PathBuf;
+
+#[rstest]
+#[case(vec!["netsuke"], PathBuf::from("Netsukefile"), None, None, Commands::Build { targets: Vec::new() })]
+#[case(
+ vec!["netsuke", "--file", "alt.yml", "-C", "work", "-j", "4", "build", "a", "b"],
+ PathBuf::from("alt.yml"),
+ Some(PathBuf::from("work")),
+ Some(4),
+ Commands::Build { targets: vec!["a".into(), "b".into()] },
+)]
+fn parse_cli(
+ #[case] argv: Vec<&str>,
+ #[case] file: PathBuf,
+ #[case] directory: Option<PathBuf>,
+ #[case] jobs: Option<usize>,
+ #[case] expected_cmd: Commands,
+) {
+ let cli = Cli::try_parse_from(argv).expect("parse");
+ assert_eq!(cli.file, file);
+ assert_eq!(cli.directory, directory);
+ assert_eq!(cli.jobs, jobs);
+ let command = cli.command.unwrap_or(Commands::Build {
+ targets: Vec::new(),
+ });
+ assert_eq!(command, expected_cmd);
+}
</code_context>
<issue_to_address>
Missing tests for error conditions and invalid CLI arguments.
Please add tests for invalid or malformed CLI arguments (e.g., unknown subcommands, missing required values, invalid types for --jobs) to verify error handling and messaging.
Suggested implementation:
```rust
use clap::error::ErrorKind;
```
```rust
#[rstest]
#[case(vec!["netsuke", "unknowncmd"], ErrorKind::UnknownArgument)]
#[case(vec!["netsuke", "--file"], ErrorKind::EmptyValue)]
#[case(vec!["netsuke", "-j", "notanumber"], ErrorKind::ValueValidation)]
#[case(vec!["netsuke", "--file", "alt.yml", "-C"], ErrorKind::EmptyValue)]
fn parse_cli_errors(
#[case] argv: Vec<&str>,
#[case] expected_error: ErrorKind,
) {
let err = Cli::try_parse_from(argv).unwrap_err();
assert_eq!(err.kind(), expected_error);
}
```
</issue_to_address>
### Comment 2
<location> `tests/steps/cli_steps.rs:45` </location>
<code_context>
+ clippy::needless_pass_by_value,
+ reason = "Cucumber requires owned String arguments"
+)]
+#[then(expr = "the manifest path is {string}")]
+fn manifest_path(world: &mut CliWorld, path: String) {
+ let cli = world.cli.as_ref().expect("cli");
+ assert_eq!(cli.file, PathBuf::from(path));
+}
+
</code_context>
<issue_to_address>
No Cucumber steps for error scenarios or invalid input.
Add Cucumber steps to test error handling for invalid or incomplete CLI arguments.
Suggested implementation:
```rust
#[allow(
clippy::needless_pass_by_value,
reason = "Cucumber requires owned String arguments"
)]
#[when(expr = "the CLI is parsed with {string}")]
fn parse_cli(world: &mut CliWorld, args: String) {
let tokens: Vec<String> = if args.is_empty() {
vec!["netsuke".to_string()]
} else {
std::iter::once("netsuke".to_string())
```
```rust
} else {
std::iter::once("netsuke".to_string())
}
.chain(args.split_whitespace().map(str::to_string))
.collect();
// Try to parse CLI, store result or error in world
match Cli::try_parse_from(&tokens) {
Ok(cli) => {
world.cli = Some(cli);
world.cli_error = None;
}
Err(e) => {
world.cli = None;
world.cli_error = Some(e.to_string());
}
}
}
/// Step: When the CLI is parsed with invalid arguments {string}
#[when(expr = "the CLI is parsed with invalid arguments {string}")]
fn parse_cli_invalid(world: &mut CliWorld, args: String) {
let tokens: Vec<String> = std::iter::once("netsuke".to_string())
.chain(args.split_whitespace().map(str::to_string))
.collect();
match Cli::try_parse_from(&tokens) {
Ok(cli) => {
world.cli = Some(cli);
world.cli_error = None;
}
Err(e) => {
world.cli = None;
world.cli_error = Some(e.to_string());
}
}
}
/// Step: Then an error should be returned
#[then("an error should be returned")]
fn error_should_be_returned(world: &mut CliWorld) {
assert!(world.cli_error.is_some(), "Expected an error, but none was returned");
}
/// Step: Then the error message should contain {string}
#[then(expr = "the error message should contain {string}")]
fn error_message_should_contain(world: &mut CliWorld, expected: String) {
let error = world.cli_error.as_ref().expect("No error was returned");
assert!(
error.contains(&expected),
"Error message '{}' does not contain expected '{}'",
error,
expected
);
}
```
- Ensure that the `CliWorld` struct has a `cli_error: Option<String>` field. If not, add it.
- Add corresponding Cucumber scenarios in your `.feature` files to use these new steps.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
Cargo.toml(2 hunks)docs/netsuke-design.md(1 hunks)docs/roadmap.md(1 hunks)src/cli.rs(1 hunks)src/lib.rs(1 hunks)src/main.rs(1 hunks)tests/cli_tests.rs(1 hunks)tests/cucumber.rs(1 hunks)tests/features/cli.feature(1 hunks)tests/steps/cli_steps.rs(1 hunks)tests/steps/mod.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
docs/**/*.md
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- AGENTS.md
**/*.md
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- AGENTS.md
⚙️ CodeRabbit Configuration File
Cargo.toml
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- AGENTS.md
**/*.rs
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- AGENTS.md
⚙️ CodeRabbit Configuration File
tests/**/*.rs
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- AGENTS.md
🧬 Code Graph Analysis (2)
tests/cli_tests.rs (1)
tests/steps/cli_steps.rs (1)
parse_cli(12-21)
tests/steps/cli_steps.rs (1)
tests/cli_tests.rs (1)
parse_cli(15-30)
🪛 LanguageTool
docs/netsuke-design.md
[grammar] ~1267-~1267: An apostrophe ‘s’ denotes possession. Did you mean to use the plural form of the noun (no apostrophe)?
Context: ...Decisions The CLI is implemented using clap's derive API in src/cli.rs. The Build...
(NOUN_APOSTROPHE_S_VERB)
[uncategorized] ~1268-~1268: Possible missing comma found.
Context: ...src/cli.rs. The Build subcommand is optional so that invoking netsuke without a su...
(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (11)
Cargo.toml (1)
54-56: Cucumber test configuration is correctly implemented.The test configuration properly disables the default harness for cucumber tests.
docs/roadmap.md (1)
15-15: Roadmap update correctly reflects CLI implementation completion.The task completion marking accurately reflects the implementation of the initial CLI structure.
tests/steps/mod.rs (1)
1-1: Module declaration follows correct patterns.The cli_steps module declaration is properly structured for organising cucumber step definitions.
src/lib.rs (1)
1-6: Library structure follows coding guidelines correctly.The module-level documentation properly explains the library's purpose and current scope. The public cli module declaration is appropriate for the current implementation phase.
tests/features/cli.feature (1)
1-12: Feature test structure follows Gherkin conventions correctly.The scenarios provide good coverage of CLI parsing functionality with clear, testable steps. The feature description and scenario names are meaningful and focused.
src/main.rs (1)
1-18: CLI entry point implementation looks correct.The main function correctly implements the CLI parsing logic as documented in the design. The default command handling using
unwrap_or(Commands::Build { targets: vec![] })properly addresses the requirement that invokingnetsukewithout a subcommand defaults to building default targets.The placeholder print statements are appropriate for this initial CLI implementation phase.
tests/cucumber.rs (1)
1-14: Cucumber test setup is well-structured.The
CliWorldstruct provides a clean way to maintain CLI state across cucumber test steps. The async main function and feature file discovery pattern follow cucumber best practices.The integration with the step definitions module creates a proper separation of concerns for behavior-driven testing.
src/cli.rs (3)
1-5: Module documentation follows coding guidelines.The module-level documentation properly explains the module's purpose and references the design documentation, adhering to the requirement that every module begin with a
//!comment.
9-27: CLI structure correctly implements the documented design.The
Clistruct accurately reflects the design specification fromdocs/netsuke-design.md. The use of-Cfor the directory flag properly mirrors Ninja's convention as documented. The default value for the manifest file matches the expected "Netsukefile" filename.
29-43: Commands enum provides the required subcommands.The
Commandsenum correctly implements the three documented subcommands:Build,Clean, andGraph. TheBuildcommand properly accepts a vector of target strings, enabling the documentednetsuke build [targets...]usage pattern.The derive attributes for
Debug,PartialEq,Eq, andClonesupport testing and comparison operations appropriately.tests/cli_tests.rs (1)
6-30: CLI test coverage is comprehensive and well-structured.The parameterised test cases using
rstestprovide excellent coverage of CLI parsing scenarios. The test validates both minimal arguments and complex combinations, ensuring the default command behaviour works correctly.The test properly validates the default command handling that matches the main function's logic using
unwrap_or(Commands::Build { targets: Vec::new() }).
|
@sourcery-ai resolve |
|
@sourcery-ai review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes - here's some feedback:
- Consider moving the CLI execution logic out of main.rs into a separate runner module so that parsing stays decoupled from business logic.
- Instead of manually unwrapping the optional subcommand in main, you can use clap’s default_subcommand attribute to handle the default
buildcommand at parse time. - The Cucumber feature tests cover basic parsing; adding scenarios for the directory (
-C) and jobs (-j) flags would help validate those options end-to-end.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider moving the CLI execution logic out of main.rs into a separate runner module so that parsing stays decoupled from business logic.
- Instead of manually unwrapping the optional subcommand in main, you can use clap’s default_subcommand attribute to handle the default `build` command at parse time.
- The Cucumber feature tests cover basic parsing; adding scenarios for the directory (`-C`) and jobs (`-j`) flags would help validate those options end-to-end.
## Individual Comments
### Comment 1
<location> `tests/features/cli.feature:2` </location>
<code_context>
+# This file is automatically @generated by Cargo.
+# It is not intended for manual editing.
+version = 4
+
+[[package]]
+name = "addr2line"
</code_context>
<issue_to_address>
Please ensure the feature file covers all subcommands and flag combinations.
Include scenarios for each subcommand and flag combination, as well as error cases like unknown subcommands or missing arguments, to ensure comprehensive CLI coverage.
</issue_to_address>
### Comment 2
<location> `tests/cucumber.rs:1` </location>
<code_context>
+use cucumber::World;
+
+#[derive(Debug, Default, World)]
</code_context>
<issue_to_address>
Consider replacing the Cucumber-based test setup with simple integration tests using assert_cmd for improved clarity and maintainability.
Consider dropping the full Cucumber setup in favor of a single integration test with assert_cmd. You can remove the `cucumber` crate, the `World` derive, your custom `main`, and the `tests/features` folder, then add something like this in `tests/cli.rs`:
```rust
// tests/cli.rs
use assert_cmd::prelude::*;
use predicates::prelude::*;
use std::process::Command;
#[test]
fn shows_help() {
let mut cmd = Command::cargo_bin("netsuke-cli").unwrap();
cmd.arg("--help");
cmd.assert()
.success()
.stdout(predicate::str::contains("Usage"));
}
#[test]
fn error_on_invalid_subcommand() {
let mut cmd = Command::cargo_bin("netsuke-cli").unwrap();
cmd.arg("no-such-cmd");
cmd.assert()
.failure()
.stderr(predicate::str::contains("unknown subcommand"));
}
```
Steps to apply:
1. Remove `cucumber = …` from Cargo.toml.
2. Delete `#[derive(World)] CliWorld`, `mod steps`, and your custom `main` in tests.
3. Create `tests/cli.rs` with `assert_cmd`-based tests as shown above.
</issue_to_address>
### Comment 3
<location> `tests/features/cli.feature:1` </location>
<code_context>
+Feature: CLI parsing
+
+ Scenario: Build is the default command
</code_context>
<issue_to_address>
Consider replacing the external Gherkin feature file with a table-driven test in Go to keep tests inline and reduce context-switching.
Consider replacing the external Gherkin feature with a small, table‐driven test in your codebase. This keeps everything in one place and avoids context‐switching:
```go
// cli_test.go
func TestCLIParsing(t *testing.T) {
tests := []struct {
name string
args []string
wantCmd string
wantManifest string
wantTarget string
wantErr bool
wantErrContains string
}{
{
name: "default to build",
args: []string{},
wantCmd: "build",
},
{
name: "override manifest and target",
args: []string{"--file", "alt.yml", "build", "target"},
wantCmd: "build",
wantManifest: "alt.yml",
wantTarget: "target",
},
{
name: "unknown command",
args: []string{"unknown"},
wantErr: true,
wantErrContains: "unknown",
},
{
name: "missing file value",
args: []string{"--file"},
wantErr: true,
wantErrContains: "--file",
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
cmd, manifest, target, err := ParseCLI(tc.args)
if tc.wantErr {
if err == nil || !strings.Contains(err.Error(), tc.wantErrContains) {
t.Fatalf("expected error containing %q, got %v", tc.wantErrContains, err)
}
return
}
if err != nil {
t.Fatal("unexpected error:", err)
}
if cmd != tc.wantCmd {
t.Errorf("cmd = %q; want %q", cmd, tc.wantCmd)
}
if manifest != tc.wantManifest {
t.Errorf("manifest = %q; want %q", manifest, tc.wantManifest)
}
if target != tc.wantTarget {
t.Errorf("target = %q; want %q", target, tc.wantTarget)
}
})
}
}
```
This keeps all parsing tests inline, reduces cognitive overhead, and covers the same scenarios without external files.
</issue_to_address>
### Comment 4
<location> `tests/steps/cli_steps.rs:17` </location>
<code_context>
+ reason = "Cucumber requires owned String arguments"
+)]
+#[when(expr = "the CLI is parsed with {string}")]
+fn parse_cli(world: &mut CliWorld, args: String) {
+ let tokens: Vec<String> = if args.is_empty() {
+ vec!["netsuke".to_string()]
</code_context>
<issue_to_address>
Consider extracting repeated CLI parsing and build command extraction logic into helper functions to reduce code duplication.
```rust
// introduce once at the top
fn apply_cli(world: &mut CliWorld, args: &str) {
let tokens = std::iter::once("netsuke".to_string())
.chain(args.split_whitespace().map(str::to_string))
.collect::<Vec<_>>();
match Cli::try_parse_from(tokens) {
Ok(cli) => {
world.cli = Some(cli);
world.cli_error = None;
}
Err(e) => {
world.cli = None;
world.cli_error = Some(e.to_string());
}
}
}
#[when(expr = "the CLI is parsed with {string}")]
fn parse_cli(world: &mut CliWorld, args: String) {
apply_cli(world, &args);
}
#[when(expr = "the CLI is parsed with invalid arguments {string}")]
fn parse_cli_invalid(world: &mut CliWorld, args: String) {
apply_cli(world, &args);
}
// helper for extracting the Build command
fn extract_build(world: &CliWorld) -> &Commands::Build {
let cmd = world
.cli
.as_ref()
.expect("cli")
.command
.as_ref()
.unwrap_or(&Commands::Build { targets: vec![] });
match cmd {
b @ Commands::Build { .. } => b,
other => panic!("Expected build command, got {:?}", other),
}
}
#[then("the command is build")]
fn command_is_build(world: &mut CliWorld) {
let _ = extract_build(world);
}
#[then(expr = "the first target is {string}")]
fn first_target(world: &mut CliWorld, target: String) {
assert_eq!(extract_build(world).targets.first(), Some(&target));
}
```
This pulls the common CLI-parsing and build-extraction logic into small helpers, removing duplicated code while keeping all existing step definitions intact.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (1)
tests/features/cli.feature (1)
1-33: Add scenarios for missing CLI features and edge cases.The feature file lacks coverage for several CLI capabilities defined in
src/cli.rs:
- Clean and Graph subcommands are not tested
- Multiple build targets scenario is missing (e.g., "build target1 target2")
- Short vs long flag variations are not validated
- Edge cases like empty string targets or invalid job counts
Add these scenarios to ensure comprehensive CLI validation:
Scenario: Clean command removes artifacts When the CLI is parsed with "clean" Then parsing succeeds And the command is clean Scenario: Graph command outputs dependency graph When the CLI is parsed with "graph" Then parsing succeeds And the command is graph Scenario: Build with multiple targets When the CLI is parsed with "build target1 target2" Then parsing succeeds And the command is build And the target list contains "target1" and "target2" Scenario: Short flags work correctly When the CLI is parsed with "-f alt.yml -C work -j 2" Then parsing succeeds And the manifest path is "alt.yml" And the working directory is "work" And the job count is 2
♻️ Duplicate comments (3)
tests/steps/cli_steps.rs (3)
69-72: Use preferred lint suppression attribute.Replace
#[allow]with#[expect]to follow the coding guidelines:-#[allow( +#[expect( clippy::needless_pass_by_value, reason = "Cucumber requires owned String arguments" )]
79-82: Use preferred lint suppression attribute.Replace
#[allow]with#[expect]to follow the coding guidelines:-#[allow( +#[expect( clippy::needless_pass_by_value, reason = "Cucumber requires owned String arguments" )]
88-91: Use preferred lint suppression attribute.Replace
#[allow]with#[expect]to follow the coding guidelines:-#[allow( +#[expect( clippy::needless_pass_by_value, reason = "Cucumber requires owned String arguments" )]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
docs/netsuke-design.md(1 hunks)docs/roadmap.md(1 hunks)src/cli.rs(1 hunks)src/lib.rs(1 hunks)src/main.rs(1 hunks)src/runner.rs(1 hunks)tests/cli_tests.rs(1 hunks)tests/features/cli.feature(1 hunks)tests/steps/cli_steps.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.rs
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- AGENTS.md
⚙️ CodeRabbit Configuration File
docs/**/*.md
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- AGENTS.md
**/*.md
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- AGENTS.md
⚙️ CodeRabbit Configuration File
tests/**/*.rs
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- AGENTS.md
🧬 Code Graph Analysis (2)
src/main.rs (2)
src/cli.rs (1)
parse_with_default(32-40)src/runner.rs (1)
run(9-23)
tests/steps/cli_steps.rs (1)
tests/cli_tests.rs (1)
parse_cli(16-38)
🪛 LanguageTool
docs/netsuke-design.md
[grammar] ~1267-~1267: An apostrophe ‘s’ denotes possession. Did you mean to use the plural form of the noun (no apostrophe)?
Context: ...Decisions The CLI is implemented using clap's derive API in src/cli.rs. Clap's `def...
(NOUN_APOSTROPHE_S_VERB)
[uncategorized] ~1268-~1268: Possible missing comma found.
Context: ... attribute marks Build as the default subcommand so invoking netsuke with no explicit ...
(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (7)
docs/roadmap.md (1)
15-17: Documentation update accurately reflects implementation completion.The roadmap correctly marks the initial CLI implementation as complete, which aligns with the actual code changes in this PR.
src/lib.rs (1)
1-7: Clean library structure with proper module documentation.The library root correctly exposes the CLI and runner modules with clear documentation that follows the required
//!format and explains the module's purpose.src/main.rs (1)
1-6: Exemplary main function implementation.The main function correctly demonstrates separation of concerns by handling only CLI parsing and delegation to the runner module. The use of
parse_with_default()ensures proper default command behavior.src/runner.rs (1)
1-23: Well-structured command dispatcher with proper documentation.The runner module correctly implements command dispatch logic with appropriate default handling and follows the required documentation format. The placeholder implementation is suitable for this initial CLI phase.
docs/netsuke-design.md (1)
1265-1274: Comprehensive CLI design documentation with accurate technical details.The new design decisions section effectively documents the CLI implementation approach, including the use of clap's derive API, default command behavior, and testing strategy. The technical details align well with the actual implementation.
Note: The possessive usage "clap's derive API" is correct as previously clarified, despite static analysis flags.
src/cli.rs (1)
1-5: Improve module documentation with examples.The module documentation lacks clear examples as required by the coding guidelines. Add practical usage examples to demonstrate CLI parsing:
//! Command line interface definition using clap. //! //! This module defines the [`Cli`] structure and its subcommands. //! It mirrors the design described in `docs/netsuke-design.md`. +//! +//! # Examples +//! +//! ``` +//! use netsuke::cli::Cli; +//! +//! // Parse CLI with default build command +//! let cli = Cli::parse_with_default(); +//! +//! // Parse specific arguments +//! let cli = Cli::try_parse_from(vec!["netsuke", "--file", "build.yml", "build", "target"]); +//! ```Likely an incorrect or invalid review comment.
tests/steps/cli_steps.rs (1)
12-31: Confirm default logic consistencyBoth apply_cli and Cli::parse_with_default check
command.is_none()and setCommands::Build { targets: Vec::new() }when none is provided. No changes required.
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
tests/steps/cli_steps.rs (3)
82-85: Use preferred lint suppression attribute.The coding guidelines forbid
#[allow]and require#[expect]with narrow scope and clear reasons.Apply this fix:
-#[allow( +#[expect( clippy::needless_pass_by_value, reason = "Cucumber requires owned String arguments" )]
92-95: Use preferred lint suppression attribute.The coding guidelines forbid
#[allow]and require#[expect]with narrow scope and clear reasons.Apply this fix:
-#[allow( +#[expect( clippy::needless_pass_by_value, reason = "Cucumber requires owned String arguments" )]
102-105: Use preferred lint suppression attribute.The coding guidelines forbid
#[allow]and require#[expect]with narrow scope and clear reasons.Apply this fix:
-#[allow( +#[expect( clippy::needless_pass_by_value, reason = "Cucumber requires owned String arguments" )]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
docs/netsuke-design.md(1 hunks)src/cli.rs(1 hunks)src/runner.rs(1 hunks)tests/cli_tests.rs(1 hunks)tests/features/cli.feature(1 hunks)tests/steps/cli_steps.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
Instructions used from:
Sources:
⚙️ CodeRabbit Configuration File
**/*.md
Instructions used from:
Sources:
⚙️ CodeRabbit Configuration File
🧬 Code Graph Analysis (1)
tests/cli_tests.rs (2)
tests/steps/cli_steps.rs (1)
parse_cli(41-43)src/cli.rs (1)
parse_from_with_default(56-64)
🪛 LanguageTool
docs/netsuke-design.md
[grammar] ~1267-~1267: An apostrophe ‘s’ denotes possession. Did you mean to use the plural form of the noun (no apostrophe)?
Context: ...Decisions The CLI is implemented using clap's derive API in src/cli.rs. Clap's `def...
(NOUN_APOSTROPHE_S_VERB)
🔇 Additional comments (13)
src/runner.rs (1)
8-23: LGTM! Clean command dispatch implementation.The function correctly implements the command dispatch pattern with appropriate default handling. The current placeholder implementation with print statements is suitable for initial development and testing.
docs/netsuke-design.md (1)
1265-1275: LGTM! Comprehensive CLI design documentation.The new subsection effectively documents the CLI implementation approach, architectural decisions, and testing strategy. The content accurately reflects the implementation and provides valuable context for future development.
tests/features/cli.feature (1)
1-60: LGTM! Comprehensive CLI test coverage.The feature file provides excellent coverage of CLI functionality with well-structured scenarios for both success and error cases. The tests appropriately validate command parsing, flag handling, and error message content.
tests/cli_tests.rs (2)
11-32: LGTM! Well-structured parameterised tests.The test function effectively covers CLI parsing success scenarios with good use of rstest parameterisation. The test cases appropriately validate different flag combinations and default behavior.
34-42: LGTM! Comprehensive error scenario testing.The error test function appropriately validates various failure conditions with correct error kind assertions. The test cases cover the main error scenarios effectively.
src/cli.rs (4)
9-21: LGTM! Robust jobs parameter validation.The validation function correctly enforces reasonable limits for the jobs parameter with clear error messages. The use of a constant for the maximum value is good practice.
23-41: LGTM! Well-structured CLI definition.The Cli struct is properly defined with appropriate clap attributes, sensible defaults, and clear documentation. The field types and validation are appropriate for the use case.
43-76: LGTM! Clean default command handling.The parsing methods are well-implemented with a functional approach. The documentation is clear, including appropriate panic conditions, and the must_use attributes are good practice.
78-92: LGTM! Clean command enum definition.The Commands enum is well-structured with appropriate derive attributes and clear documentation. The use of unit structs for parameterless commands is idiomatic Rust.
tests/steps/cli_steps.rs (4)
1-10: Module documentation and imports look excellent.The module documentation follows the required format and clearly explains the purpose. The imports are well-organised and appropriate for the functionality.
36-52: CLI parsing step definitions are well-implemented.The functions properly use
#[expect]for lint suppression with clear reasons and delegate to theapply_clihelper function, reducing code duplication.
54-80: Command validation steps are well-structured.The functions use appropriate helper functions, idiomatic Rust patterns like
matches!, and follow the coding guidelines by using.expect()over.unwrap().
118-137: Error handling steps are well-implemented.The functions use descriptive assertion messages and follow the coding guidelines with proper lint suppression using
#[expect].
1 participant
Summary
Testing
make fmtmake lintmake testmake markdownlinthttps://chatgpt.com/codex/tasks/task_e_6876a8d8fc888322b6ce54e053fb13e3
Summary by Sourcery
Implement the initial Clap-based command-line interface with build, clean, and graph subcommands, add comprehensive unit and BDD tests, and update documentation and project configuration accordingly.
New Features:
Build:
Documentation:
Tests: