Complete CLI config struct implementation with OrthoConfig (0.8.0)#273
Complete CLI config struct implementation with OrthoConfig (0.8.0)#273leynos wants to merge 19 commits into
Conversation
Reviewer's GuideRefactors the CLI into separate parser, config, and merge layers driven by a new OrthoConfig-derived CliConfig schema, wires layered configuration (including build subcommand defaults and typed presentation enums) through this path, upgrades ortho_config to 0.8.0, and updates tests and documentation to match the new precedence and configuration model. Sequence diagram for CLI parse and layered configuration mergesequenceDiagram
actor User
participant Main as main_rs
participant Parser as parser_Cli
participant Merge as merge_module
participant Ortho as OrthoConfig_runtime
participant Runner as runner_run
User->>Main: invoke_netsuke(args)
Main->>Parser: parse_with_localizer_from(args, localizer)
Parser->>Parser: configure_validation_parsers()
Parser->>Parser: Cli::from_arg_matches_mut()
Parser-->>Main: (parsed_cli Cli, matches ArgMatches)
Main->>Merge: merge_with_config(parsed_cli, matches)
Merge->>Merge: config_discovery(directory)
Merge->>Ortho: compose_layers(defaults, files, env, cli_overrides)
Ortho-->>Merge: LayerComposition
Merge->>Ortho: CliConfig::merge_from_layers(LayerComposition)
Ortho-->>Merge: merged_config CliConfig
Merge->>Merge: apply_config(parsed_cli, merged_config)
Merge-->>Main: merged_cli Cli
Main->>Main: init_tracing(merged_cli.verbose)
Main->>Main: prefs = output_prefs::resolve(merged_cli.no_emoji_override())
Main->>Runner: run(&merged_cli, prefs)
Runner->>Runner: progress_enabled = merged_cli.progress_enabled()
Runner-->>Main: Result
Main-->>User: ExitCode
Class diagram for layered CLI configuration (CliConfig, parser Cli, and merge)classDiagram
class Cli {
+PathBuf file
+Option_PathBuf_ directory
+Option_usize_ jobs
+bool verbose
+Option_String_ locale
+Vec_String_ fetch_allow_scheme
+Vec_HostPattern_ fetch_allow_host
+Vec_HostPattern_ fetch_block_host
+bool fetch_default_deny
+Option_bool_ accessible
+Option_bool_ no_emoji
+Option_bool_ progress
+Option_ColourPolicy_ colour_policy
+Option_SpinnerMode_ spinner_mode
+Option_OutputFormat_ output_format
+Option_Theme_ theme
+Option_Commands_ command
+with_default_command() Cli
+no_emoji_override() Option_bool_
+progress_enabled() bool
}
class Commands {
<<enumeration>>
Build
Clean
Graph
Manifest
}
class BuildArgs {
+Option_PathBuf_ emit
+Vec_String_ targets
}
class CliConfig {
+PathBuf file
+Option_usize_ jobs
+bool verbose
+Option_String_ locale
+Vec_String_ fetch_allow_scheme
+Vec_HostPattern_ fetch_allow_host
+Vec_HostPattern_ fetch_block_host
+bool fetch_default_deny
+Option_bool_ accessible
+Option_bool_ no_emoji
+Option_bool_ progress
+Option_ColourPolicy_ colour_policy
+Option_SpinnerMode_ spinner_mode
+Option_OutputFormat_ output_format
+Option_Theme_ theme
+CommandConfigs cmds
+post_merge(ctx PostMergeContext) OrthoResult~()~
}
class CommandConfigs {
+BuildConfig build
}
class BuildConfig {
+Option_PathBuf_ emit
+Vec_String_ targets
}
class ColourPolicy {
<<enumeration>>
Auto
Always
Never
}
class SpinnerMode {
<<enumeration>>
Auto
Enabled
Disabled
}
class OutputFormat {
<<enumeration>>
Human
Json
}
class Theme {
<<enumeration>>
Auto
Unicode
Ascii
}
class MergeModule {
+merge_with_config(cli Cli, matches ArgMatches) OrthoResult~Cli~
+apply_config(parsed Cli, config CliConfig) Cli
+resolve_command(parsed Option_Commands_, build_defaults BuildConfig) Commands
+canonical_theme(theme Option_Theme_, no_emoji Option_bool_) Option_Theme_
}
class ParserModule {
+parse_with_localizer_from(iter I, localizer Arc_Localizer_) Result~(Cli,ArgMatches),clap::Error~
+validation_message(localizer Localizer, key str, args Option_LocalizationArgs_, fallback str) String
}
Cli --> Commands : uses
Commands --> BuildArgs : Build(BuildArgs)
CliConfig --> CommandConfigs : has
CommandConfigs --> BuildConfig : has
Cli --> ColourPolicy : uses
Cli --> SpinnerMode : uses
Cli --> OutputFormat : uses
Cli --> Theme : uses
CliConfig --> ColourPolicy : uses
CliConfig --> SpinnerMode : uses
CliConfig --> OutputFormat : uses
CliConfig --> Theme : uses
MergeModule --> CliConfig : merges
MergeModule --> Cli : produces
ParserModule --> Cli : parses
ParserModule ..> MergeModule : passes Cli, ArgMatches
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Caution Review failedFailed to post review comments WalkthroughIntroduce an OrthoConfig-derived ChangesCliConfig and CLI refactor
Sequence Diagram(s)sequenceDiagram
participant User
participant Parser as src/cli/parser.rs
participant Merge as src/cli/merge.rs
participant Config as ConfigDiscovery
participant Env as Environment
participant Runtime as RuntimeStartup
User->>Parser: supply CLI args
Parser->>Merge: (Cli, ArgMatches)
Merge->>Config: discover/load file layers
Env-->>Merge: push NETSUKE_* env layer
Merge->>Merge: compose defaults + file + env + CLI overrides
Merge->>Runtime: apply merged CliConfig → runtime Cli
Possibly related PRs
Suggested labels
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (4 errors, 9 warnings, 1 inconclusive)
✅ Passed checks (6 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
changed the title
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on file fn cli_config_build_defaults_apply_when_cli_targets_are_absent() -> Result<()> {
let _env_lock = EnvLock::acquire();
let temp_dir = tempdir().context("create temporary config directory")?;
let config_path = temp_dir.path().join("netsuke.toml");
fs::write(
&config_path,
r#"
[cmds.build]
targets = ["all", "docs"]
"#,
)
.context("write netsuke.toml")?;
let _config_guard = EnvVarGuard::set("NETSUKE_CONFIG_PATH", config_path.as_os_str());
let localizer = Arc::from(cli_localization::build_localizer(None));
let (cli, matches) = netsuke::cli::parse_with_localizer_from(["netsuke"], &localizer)
.context("parse CLI args for merge")?;
let merged = netsuke::cli::merge_with_config(&cli, &matches)
.context("merge CLI and configuration layers")?;
let Some(netsuke::cli::Commands::Build(args)) = merged.command else {
anyhow::bail!("expected merged command to be build");
};
ensure!(
args.targets == vec![String::from("all"), String::from("docs")],
"configured build targets should be used when CLI targets are absent",
);
Ok(())
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on file fn cli_config_build_defaults_apply_when_cli_targets_are_absent() -> Result<()> {
with_config_file(
r#"
[cmds.build]
targets = ["all", "docs"]
"#,
&["netsuke"],
|merged| {
let Some(netsuke::cli::Commands::Build(args)) = merged.command else {
anyhow::bail!("expected merged command to be build");
};
ensure!(
args.targets == vec![String::from("all"), String::from("docs")],
"configured build targets should be used when CLI targets are absent",
);
Ok(())
},
)
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on file fn cli_config_validates_theme_alias_conflicts(
default_cli_json: Result<serde_json::Value>,
) -> Result<()> {
assert_merge_rejects(
default_cli_json?,
json!({
"theme": "unicode",
"no_emoji": true
}),
"conflicting theme and alias should fail",
"theme = \"unicode\" conflicts with no_emoji = true",
)
}❌ New issue: Code Duplication |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on file use anyhow::{Context, Result, ensure};
use netsuke::cli::Cli;
use netsuke::cli::{CliConfig, Theme};❌ New issue: String Heavy Function Arguments |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on file use anyhow::{Context, Result, ensure};
use netsuke::cli::Cli;
use netsuke::cli::{CliConfig, Theme};❌ New issue: String Heavy Function Arguments |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf2ccff65d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai Please investigate the cause of the following issue using codegraph exploration and research, identify a fix and provide an AI coding agent prompt for the fix: |
Replace the two string parameters on parse_value_enum with a single spec struct so call sites pass one grouped argument and CodeScene's string-heavy argument metric drops below threshold. Co-authored-by: Cursor <cursoragent@cursor.com>
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Move file-layer and environment branching out of resolve_merged_diag_json into diag_json_from_file_layers and diag_json_from_env so the public entry point delegates through three focused steps and cyclomatic complexity drops. Co-authored-by: Cursor <cursoragent@cursor.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Resolve explicit and discovered config paths through map_or_else so a single Ok/Err match pushes layers or records one error, removing the duplicated nested match arms. Co-authored-by: Cursor <cursoragent@cursor.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Add package metadata pointing cargo-orthohelp at CliConfig and the supported locale list so release help generation can resolve the schema. Co-authored-by: Cursor <cursoragent@cursor.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. Code Duplicationsrc/cli/parsing.rs: What lead to degradation?The module contains 3 functions with similar structure: parse_colour_policy,parse_output_format,parse_spinner_mode Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. String Heavy Function Argumentssrc/cli/parsing.rs: What lead to degradation?In this module, 47.4% of all arguments to its 9 functions are strings. The threshold for string arguments is 39.0% Why does this problem occur?String is a generic type that fail to capture the constraints of the domain object it represents. In this module, 47 % of all function arguments are string types. How to fix it?Heavy string usage indicates a missing domain language. Introduce data types that encapsulate the semantics. For example, a user_name is better represented as a constrained User type rather than a pure string, which could be anything. |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. String Heavy Function Argumentstests/cli_tests/merge.rs: What lead to degradation?In this module, 46.2% of all arguments to its 11 functions are strings. The threshold for string arguments is 39.0% Why does this problem occur?String is a generic type that fail to capture the constraints of the domain object it represents. In this module, 46 % of all function arguments are string types. How to fix it?Heavy string usage indicates a missing domain language. Introduce data types that encapsulate the semantics. For example, a user_name is better represented as a constrained User type rather than a pure string, which could be anything. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== src/cli/parsing.rs: full file ==="
fd -e rs parsing src/cli/ --exec cat {}Length of output: 6003 File: Assessment: suppress this diagnosticFurther refactoring here would be deleterious. Apply the suppression text below. Suppression justificationThe All nine functions are |
Replace string expected_msg parameters with ExpectedValidationError so assert_merge_rejects and its rstest cases carry typed expectations and CodeScene's string-argument ratio drops below threshold. Co-authored-by: Cursor <cursoragent@cursor.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for the following concern and provide an AI coding agent prompt for the fix: The branch leaves old-looking files such as I would treat the branch as a substantive improvement over |
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Summary
no_emoji.cmds.build.targets) through the layered config path.Changes
New modules and schema:
Refactor: moved merge logic and OrthoConfig-driven flow out of src/cli/mod.rs into dedicated modules for maintainability and testability.
Parser/merge wiring: CliConfig is wired into the OrthoConfig discovery/merge path so global values and subcommand defaults (e.g. cmds.build.targets) resolve through the same typed fields.
Compatibility: preserve hidden config-path behavior and keep --config handling out of scope for this milestone.
Tooling and dependencies:
Documentation and tests:
Example config (TOML) (for reference):
verbose = true
locale = "es-ES"
colour_policy = "auto"
spinner_mode = "auto"
output_format = "human"
theme = "ascii"
progress = true
accessible = false
[cmds.build]
targets = ["all"]
This demonstrates a canonical view where global fields live at the root and subcommand defaults (like build targets) live under their respective sections.
Documentation impact
How to test locally
Plan alignment and next steps
Idempotence and recovery
📎 Task: https://www.devboxer.com/task/c5d87114-1d80-439d-b1a9-21848725bc15