Add explicit CLI overrides with typed enums; update tests/docs/build #276
+203
−37
There are no files selected for viewing
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
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
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,7 @@ use crate::bdd::fixtures::{RefCellOptionExt, TestWorld}; | |
| use crate::bdd::helpers::parse_store::store_parse_outcome; | ||
| use crate::bdd::helpers::tokens::build_tokens; | ||
| use anyhow::{Context, Result, anyhow, bail, ensure}; | ||
| use netsuke::cli::{Cli, ColourPolicy, Commands, SpinnerMode, Theme}; | ||
| use netsuke::cli_localization; | ||
| use netsuke::output_prefs; | ||
| use rstest_bdd_macros::{given, then, when}; | ||
|
|
@@ -39,7 +39,7 @@ fn write_config(world: &TestWorld, contents: &str) -> Result<()> { | |
| 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()) }; | ||
|
Comment on lines
41
to
+42
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:
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:
|
||
| world.track_env_var(CONFIG_ENV_VAR.to_owned(), previous); | ||
| Ok(()) | ||
| } | ||
|
|
@@ -95,6 +95,63 @@ fn config_sets_no_emoji(world: &TestWorld) -> Result<()> { | |
| write_config(world, "no_emoji = true\n") | ||
| } | ||
|
|
||
| #[given("the Netsuke config file sets theme to {theme:string}")] | ||
| fn config_sets_theme(world: &TestWorld, theme: &str) -> Result<()> { | ||
| write_config(world, &format!("theme = \"{theme}\"\n")) | ||
| } | ||
|
|
||
| #[given("the Netsuke config file sets colour policy to {policy:string}")] | ||
| fn config_sets_colour_policy(world: &TestWorld, policy: &str) -> Result<()> { | ||
| write_config(world, &format!("colour_policy = \"{policy}\"\n")) | ||
| } | ||
|
|
||
| #[given("the Netsuke config file sets spinner mode to {mode:string}")] | ||
| fn config_sets_spinner_mode(world: &TestWorld, mode: &str) -> Result<()> { | ||
| write_config(world, &format!("spinner_mode = \"{mode}\"\n")) | ||
| } | ||
|
|
||
| #[given("the NETSUKE_THEME environment variable is {theme:string}")] | ||
| #[expect( | ||
| clippy::unnecessary_wraps, | ||
| reason = "rstest-bdd macro generates Result wrapper; FIXME: https://github.com/leynos/rstest-bdd/issues/381" | ||
| )] | ||
| fn set_environment_theme_override(world: &TestWorld, theme: &str) -> Result<()> { | ||
| ensure_env_lock(world); | ||
| let previous = std::env::var_os("NETSUKE_THEME"); | ||
| // SAFETY: `EnvLock` is held in `world.env_lock` for the lifetime of the scenario. | ||
| unsafe { std::env::set_var("NETSUKE_THEME", OsStr::new(theme)) }; | ||
| world.track_env_var("NETSUKE_THEME".to_owned(), previous); | ||
| Ok(()) | ||
| } | ||
|
|
||
| #[given("the NETSUKE_COLOUR_POLICY environment variable is {policy:string}")] | ||
| #[expect( | ||
| clippy::unnecessary_wraps, | ||
| reason = "rstest-bdd macro generates Result wrapper; FIXME: https://github.com/leynos/rstest-bdd/issues/381" | ||
| )] | ||
| fn set_environment_colour_policy_override(world: &TestWorld, policy: &str) -> Result<()> { | ||
| ensure_env_lock(world); | ||
| let previous = std::env::var_os("NETSUKE_COLOUR_POLICY"); | ||
| // SAFETY: `EnvLock` is held in `world.env_lock` for the lifetime of the scenario. | ||
| unsafe { std::env::set_var("NETSUKE_COLOUR_POLICY", OsStr::new(policy)) }; | ||
| world.track_env_var("NETSUKE_COLOUR_POLICY".to_owned(), previous); | ||
| Ok(()) | ||
| } | ||
|
|
||
| #[given("the NETSUKE_SPINNER_MODE environment variable is {mode:string}")] | ||
| #[expect( | ||
| clippy::unnecessary_wraps, | ||
| reason = "rstest-bdd macro generates Result wrapper; FIXME: https://github.com/leynos/rstest-bdd/issues/381" | ||
| )] | ||
| fn set_environment_spinner_mode_override(world: &TestWorld, mode: &str) -> Result<()> { | ||
| ensure_env_lock(world); | ||
| let previous = std::env::var_os("NETSUKE_SPINNER_MODE"); | ||
| // SAFETY: `EnvLock` is held in `world.env_lock` for the lifetime of the scenario. | ||
| unsafe { std::env::set_var("NETSUKE_SPINNER_MODE", OsStr::new(mode)) }; | ||
| world.track_env_var("NETSUKE_SPINNER_MODE".to_owned(), previous); | ||
| Ok(()) | ||
| } | ||
|
|
||
| #[expect( | ||
| clippy::unnecessary_wraps, | ||
| reason = "rstest-bdd macro generates Result wrapper; FIXME: https://github.com/leynos/rstest-bdd/issues/381" | ||
|
|
@@ -181,3 +238,42 @@ fn merge_error_contains(world: &TestWorld, fragment: &str) -> Result<()> { | |
| ); | ||
| Ok(()) | ||
| } | ||
|
|
||
| #[then("the merged theme is unicode")] | ||
| fn merged_theme_is_unicode(world: &TestWorld) -> Result<()> { | ||
| let theme = world | ||
| .cli | ||
| .with_ref(|cli| cli.theme) | ||
| .context("expected merged CLI to be available")?; | ||
| ensure!( | ||
| theme == Some(Theme::Unicode), | ||
| "expected merged theme to be Unicode, got {theme:?}", | ||
| ); | ||
| Ok(()) | ||
| } | ||
|
|
||
| #[then("the merged colour policy is always")] | ||
| fn merged_colour_policy_is_always(world: &TestWorld) -> Result<()> { | ||
| let policy = world | ||
| .cli | ||
| .with_ref(|cli| cli.colour_policy) | ||
| .context("expected merged CLI to be available")?; | ||
| ensure!( | ||
| policy == Some(ColourPolicy::Always), | ||
| "expected merged colour policy to be Always, got {policy:?}", | ||
| ); | ||
| Ok(()) | ||
| } | ||
|
|
||
| #[then("the merged spinner mode is enabled")] | ||
| 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(()) | ||
| } | ||
|
Comment on lines
+269
to
+279
There was a problem hiding this comment. ❌ New issue: Code Duplication |
||
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ 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%
Suppress