Add explicit CLI overrides with typed enums; update tests/docs/build#276
Open
leynos wants to merge 4 commits intoadd-cli-config-struct-vcf3xyfrom
Open
Add explicit CLI overrides with typed enums; update tests/docs/build#276leynos wants to merge 4 commits intoadd-cli-config-struct-vcf3xyfrom
leynos wants to merge 4 commits intoadd-cli-config-struct-vcf3xyfrom
Conversation
Refactored environment variable setting in BDD configuration preference tests by introducing EnvVarGuard to manage environment state more safely. This replaces unsafe direct std::env::set_var calls and improves test reliability. Additionally, minor spacing cleanup was done in documentation files. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Reviewer's GuideImplements explicit, typed CLI override flags for visual output settings and wires them into the configuration merge path, adds ValueEnum support for those enums, refactors the build script into clearer phases, and tightens test env-var handling plus minor doc formatting/clarity tweaks. Sequence diagram for explicit CLI overrides in configuration mergesequenceDiagram
actor User
participant CliParser as CliParser
participant ArgMatches as ArgMatches
participant CliMerge as CliMerge
participant CliConfig as CliConfig
User->>CliParser: netsuke --colour-policy POLICY --spinner-mode MODE --output-format FORMAT --theme THEME
CliParser->>CliParser: Parse options into Cli
CliParser->>ArgMatches: Build ArgMatches
CliParser->>CliMerge: cli_overrides_from_matches(&Cli,&ArgMatches)
CliMerge->>CliMerge: maybe_insert_explicit(progress,no_emoji,diag_json,...)
CliMerge->>CliMerge: maybe_insert_explicit(colour_policy,spinner_mode,output_format,theme)
CliMerge->>CliConfig: Produce ValueMap for CLI overrides
CliConfig->>CliConfig: Layer defaults
CliConfig->>CliConfig: Apply config files
CliConfig->>CliConfig: Apply env vars
CliConfig->>CliConfig: Apply CLI overrides (typed enums)
CliConfig-->>User: Resolved runtime configuration
Updated class diagram for CLI types and visual configuration enumsclassDiagram
class Cli {
+Option~bool~ progress
+Option~ColourPolicy~ colour_policy
+Option~SpinnerMode~ spinner_mode
+Option~OutputFormat~ output_format
+Option~Theme~ theme
+Option~Commands~ command
}
class ColourPolicy {
<<enum ValueEnum>>
+Environment
+Always
+Never
}
class SpinnerMode {
<<enum ValueEnum>>
+Auto
+Always
+Never
}
class OutputFormat {
<<enum ValueEnum>>
+Human
+Json
+Silent
}
class Theme {
<<enum ValueEnum>>
+Environment
+Light
+Dark
}
class CliConfig {
+ColourPolicy colour_policy
+SpinnerMode spinner_mode
+OutputFormat output_format
+Theme theme
}
Cli --> ColourPolicy
Cli --> SpinnerMode
Cli --> OutputFormat
Cli --> Theme
CliConfig --> ColourPolicy
CliConfig --> SpinnerMode
CliConfig --> OutputFormat
CliConfig --> Theme
Flow diagram for refactored build script phasesflowchart TD
Start(["cargo build invokes main"]) --> VerifyPublicApi["verify_public_api_symbols"]
VerifyPublicApi --> EmitRerun["emit_rerun_directives"]
EmitRerun --> AuditL10n["build_l10n_audit::audit_localization_keys"]
AuditL10n --> OutDir["out_dir_for_target_profile"]
OutDir --> GenMan["generate_man_page"]
GenMan --> WritePrimary["write_man_page to target/generated-man"]
GenMan --> WriteOutDir["write_man_page to OUT_DIR (best-effort)"]
WritePrimary --> End(["Build completes"])
WriteOutDir --> End
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.
Add support for overriding colour policy, spinner mode, output format, and theme via CLI options. Derive ValueEnum for these enums to enable command line parsing integration. Update config merging logic to respect these new CLI flags. Refactor build script by splitting main function into smaller functions for clarity and maintainability. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
changed the title
…safe Replaced usage of the EnvVarGuard helper with direct calls to std::env::set_var inside unsafe blocks in BDD test steps. This simplifies environment variable management by manually tracking previous values without using the guard abstraction. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
leynos
changed the title
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}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tests/bdd/steps/configuration_preferences.rs" line_range="41-42" />
<code_context>
fs::write(&path, contents).with_context(|| format!("write {}", path.display()))?;
let previous = std::env::var_os(CONFIG_ENV_VAR);
// SAFETY: `EnvLock` is held in `world.env_lock` for the lifetime of the scenario.
- unsafe { std::env::set_var(CONFIG_ENV_VAR, OsStr::new(path.as_os_str())) };
+ unsafe { std::env::set_var(CONFIG_ENV_VAR, path.as_os_str()) };
world.track_env_var(CONFIG_ENV_VAR.to_owned(), previous);
Ok(())
</code_context>
<issue_to_address>
**suggestion (testing):** Add BDD coverage for the new explicit CLI override flags and their interaction with configuration and environment variables.
This file already exercises configuration and env-var wiring, so it’s a good place to assert the new CLI override behavior. Please add or extend BDD scenarios to:
- Run the CLI with each new flag and assert the merged configuration uses the CLI value.
- Assert precedence when both config and CLI specify a value (CLI must win).
- Where applicable, cover env + config + CLI together, checking that CLI still wins and that env isolation holds between scenarios.
This coverage will more firmly validate the new merge/precedence rules and protect against regressions for these enum-based flags.
Suggested implementation:
```rust
fs::write(&path, contents).with_context(|| format!("write {}", path.display()))?;
let previous = std::env::var_os(CONFIG_ENV_VAR);
// SAFETY: `EnvLock` is held in `world.env_lock` for the lifetime of the scenario.
unsafe { std::env::set_var(CONFIG_ENV_VAR, path.as_os_str()) };
world.track_env_var(CONFIG_ENV_VAR.to_owned(), previous);
Ok(())
}
///
/// BDD step definitions to validate CLI override flags and their precedence
/// against configuration files and environment variables.
///
/// These steps are intended to be reused across scenarios that:
/// - set configuration values via config file
/// - optionally set environment variables
/// - provide explicit CLI override flags
/// and then assert that the merged configuration prefers CLI over env and config.
///
/// Record a CLI override flag value in the world before resolving configuration.
///
/// Example Gherkin:
/// Given the CLI is run with the "mode" override flag set to "fast"
#[given(regex = r#"^the CLI is run with the "([^"]+)" override flag set to "([^"]+)"$"#)]
async fn given_cli_override_flag(world: &mut ConfigurationWorld, flag: String, value: String) {
world.cli_overrides.insert(flag, value);
}
/// Trigger configuration resolution using the currently configured config file,
/// environment variables, and any registered CLI overrides.
///
/// Example Gherkin:
/// When the configuration is resolved with CLI, config file, and environment variables
#[when("the configuration is resolved with CLI, config file, and environment variables")]
async fn when_configuration_is_resolved(world: &mut ConfigurationWorld) {
// This method should:
// - apply config file values
// - overlay environment variables
// - finally overlay explicit CLI overrides so that CLI wins
world
.resolve_configuration()
.await
.expect("configuration resolution failed");
}
/// Assert the final effective configuration value for a given key, ensuring
/// that CLI override precedence is honored over env and config.
///
/// Example Gherkin:
/// Then the effective configuration value for "mode" is "fast"
#[then(regex = r#"^the effective configuration value for "([^"]+)" is "([^"]+)"$"#)]
async fn then_effective_configuration_uses_cli_value(
world: &mut ConfigurationWorld,
key: String,
expected: String,
) {
let actual = world
.effective_configuration
.get(&key)
.unwrap_or_else(|| panic!("missing configuration key {key}"));
assert_eq!(
actual, &expected,
"expected effective configuration[{key}] = {expected}, got {actual}"
);
}
```
To fully implement the requested BDD coverage, the following additional work is likely required:
1. Ensure `ConfigurationWorld`:
- Is the actual world type used in this file (if it has a different name, update the step signatures accordingly).
- Exposes a `cli_overrides: HashMap<String, String>` or similar field; initialize it in the world constructor/reset logic.
- Exposes an `effective_configuration: HashMap<String, String>` (or appropriate type) populated by `resolve_configuration`.
- Provides an async `resolve_configuration(&mut self)` method that merges configuration as:
config file < environment variables < explicit CLI overrides.
If a differently named method already exists that performs this merge, adapt the call accordingly.
2. Wire these steps into your existing BDD setup:
- If you are using a `steps!` or similar registration macro/module, add these three step functions there so they are discoverable.
- Make sure the `#[given]`, `#[when]`, and `#[then]` attribute macros and `ConfigurationWorld` are imported at the top of this file, matching your existing conventions.
3. Add/extend `.feature` files to use these steps:
- Scenarios that set only config + CLI and assert CLI wins.
- Scenarios that set env + config + CLI and assert CLI still wins and env isolation holds between scenarios.
- For each new explicit CLI override flag (enum-based flags), add scenarios that:
* set the flag only via CLI and assert the effective configuration.
* set conflicting values via config and CLI and assert the CLI value is used.
* optionally combine env + config + CLI for that flag to validate full precedence.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
41
to
+42
| // SAFETY: `EnvLock` is held in `world.env_lock` for the lifetime of the scenario. | ||
| unsafe { std::env::set_var(CONFIG_ENV_VAR, OsStr::new(path.as_os_str())) }; | ||
| unsafe { std::env::set_var(CONFIG_ENV_VAR, path.as_os_str()) }; |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add BDD coverage for the new explicit CLI override flags and their interaction with configuration and environment variables.
This file already exercises configuration and env-var wiring, so it’s a good place to assert the new CLI override behavior. Please add or extend BDD scenarios to:
- Run the CLI with each new flag and assert the merged configuration uses the CLI value.
- Assert precedence when both config and CLI specify a value (CLI must win).
- Where applicable, cover env + config + CLI together, checking that CLI still wins and that env isolation holds between scenarios.
This coverage will more firmly validate the new merge/precedence rules and protect against regressions for these enum-based flags.
Suggested implementation:
fs::write(&path, contents).with_context(|| format!("write {}", path.display()))?;
let previous = std::env::var_os(CONFIG_ENV_VAR);
// SAFETY: `EnvLock` is held in `world.env_lock` for the lifetime of the scenario.
unsafe { std::env::set_var(CONFIG_ENV_VAR, path.as_os_str()) };
world.track_env_var(CONFIG_ENV_VAR.to_owned(), previous);
Ok(())
}
///
/// BDD step definitions to validate CLI override flags and their precedence
/// against configuration files and environment variables.
///
/// These steps are intended to be reused across scenarios that:
/// - set configuration values via config file
/// - optionally set environment variables
/// - provide explicit CLI override flags
/// and then assert that the merged configuration prefers CLI over env and config.
///
/// Record a CLI override flag value in the world before resolving configuration.
///
/// Example Gherkin:
/// Given the CLI is run with the "mode" override flag set to "fast"
#[given(regex = r#"^the CLI is run with the "([^"]+)" override flag set to "([^"]+)"$"#)]
async fn given_cli_override_flag(world: &mut ConfigurationWorld, flag: String, value: String) {
world.cli_overrides.insert(flag, value);
}
/// Trigger configuration resolution using the currently configured config file,
/// environment variables, and any registered CLI overrides.
///
/// Example Gherkin:
/// When the configuration is resolved with CLI, config file, and environment variables
#[when("the configuration is resolved with CLI, config file, and environment variables")]
async fn when_configuration_is_resolved(world: &mut ConfigurationWorld) {
// This method should:
// - apply config file values
// - overlay environment variables
// - finally overlay explicit CLI overrides so that CLI wins
world
.resolve_configuration()
.await
.expect("configuration resolution failed");
}
/// Assert the final effective configuration value for a given key, ensuring
/// that CLI override precedence is honored over env and config.
///
/// Example Gherkin:
/// Then the effective configuration value for "mode" is "fast"
#[then(regex = r#"^the effective configuration value for "([^"]+)" is "([^"]+)"$"#)]
async fn then_effective_configuration_uses_cli_value(
world: &mut ConfigurationWorld,
key: String,
expected: String,
) {
let actual = world
.effective_configuration
.get(&key)
.unwrap_or_else(|| panic!("missing configuration key {key}"));
assert_eq!(
actual, &expected,
"expected effective configuration[{key}] = {expected}, got {actual}"
);
}To fully implement the requested BDD coverage, the following additional work is likely required:
-
Ensure
ConfigurationWorld:- Is the actual world type used in this file (if it has a different name, update the step signatures accordingly).
- Exposes a
cli_overrides: HashMap<String, String>or similar field; initialize it in the world constructor/reset logic. - Exposes an
effective_configuration: HashMap<String, String>(or appropriate type) populated byresolve_configuration. - Provides an async
resolve_configuration(&mut self)method that merges configuration as:
config file < environment variables < explicit CLI overrides.
If a differently named method already exists that performs this merge, adapt the call accordingly.
-
Wire these steps into your existing BDD setup:
- If you are using a
steps!or similar registration macro/module, add these three step functions there so they are discoverable. - Make sure the
#[given],#[when], and#[then]attribute macros andConfigurationWorldare imported at the top of this file, matching your existing conventions.
- If you are using a
-
Add/extend
.featurefiles to use these steps:- Scenarios that set only config + CLI and assert CLI wins.
- Scenarios that set env + config + CLI and assert CLI still wins and env isolation holds between scenarios.
- For each new explicit CLI override flag (enum-based flags), add scenarios that:
- set the flag only via CLI and assert the effective configuration.
- set conflicting values via config and CLI and assert the CLI value is used.
- optionally combine env + config + CLI for that flag to validate full precedence.
…ner mode precedence - Added various BDD step definitions for setting and verifying configuration preferences - Covered config file, environment variable, and CLI flag precedence for theme, colour policy, and spinner mode - Extended feature file with scenarios validating correct merge and override behavior among sources Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
There was a problem hiding this comment.
(1 files improve in Code Health)
Gates Failed
(1 file with Code Duplication, String Heavy Function Arguments)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| configuration_preferences.rs | 2 advisory rules | 10.00 → 9.10 | Suppress |
View Improvements
| File | Code Health Impact | Categories Improved |
|---|---|---|
| build.rs | 9.61 → 10.00 | Large Method |
Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
Comment on lines
+269
to
+279
| fn merged_spinner_mode_is_enabled(world: &TestWorld) -> Result<()> { | ||
| let mode = world | ||
| .cli | ||
| .with_ref(|cli| cli.spinner_mode) | ||
| .context("expected merged CLI to be available")?; | ||
| ensure!( | ||
| mode == Some(SpinnerMode::Enabled), | ||
| "expected merged spinner mode to be Enabled, got {mode:?}", | ||
| ); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
❌ New issue: Code Duplication
The module contains 4 functions with similar structure: merged_colour_policy_is_always,merged_spinner_mode_is_enabled,merged_theme_is_ascii,merged_theme_is_unicode
| use crate::bdd::helpers::tokens::build_tokens; | ||
| use anyhow::{Context, Result, anyhow, bail, ensure}; | ||
| use netsuke::cli::{Cli, Commands, Theme}; | ||
| use netsuke::cli::{Cli, ColourPolicy, Commands, SpinnerMode, Theme}; |
There was a problem hiding this comment.
❌ New issue: String Heavy Function Arguments
In this module, 39.0% of all arguments to its 25 functions are strings. The threshold for string arguments is 39.0%
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
1 participant
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
Changes
CLI
src/cli/merge.rs: insert explicit CLI flag mappings forcolour_policy,spinner_mode,output_format, andthemeinto the root merge data so CLI-provided overrides are captured during merge.Parser
src/cli/parser.rs: convert the explicit override fields to be override-enabled CLI flags (long options) with appropriate value names:POLICY,MODE,FORMAT, andTHEME.Config
src/cli/config.rs: deriveValueEnumforColourPolicy,SpinnerMode,OutputFormat, andThemeto support explicit CLI overrides with typed values.Testing
tests/bdd/steps/configuration_preferences.rs:test_support::EnvVarGuardforCONFIG_ENV_VARandLOCALE_ENV_VAR.guard.original()and restore it via world tracking.std::env::set_varcalls with guarded variable setting to improve safety and determinism in tests.Build Script
build.rs:Documentation
Test plan
cargo test.Rationale
ValueEnumenables safe, typed CLI override values.EnvVarGuardfor tests improves isolation and safety by avoiding unsafe global env mutations.📎 Task: https://www.devboxer.com/task/a7003265-1e31-47cd-a55c-63d4846862b7
Summary by Sourcery
Introduce explicit typed CLI overrides for output-related settings and refactor build-time tooling for clearer responsibilities and man-page generation.
New Features:
Enhancements:
ValueEnumfor output-related configuration enums to support typed CLI parsing.Tests: