Add Command::next_env_prefix for env variable prefixing#6281
Add Command::next_env_prefix for env variable prefixing#6281veeceey wants to merge 1 commit intoclap-rs:masterfrom
Conversation
|
Test results (all env tests pass, including 5 new env_prefix tests): |
|
We recommend that commits represent how things should be reviewed and merged and not how they were developed. Having fixup commits means they are not atomic. There is also a strong preference for adding tests in a previous commit, with them passing, showing the current behavior. How to handle this when its a new API is context dependent. In this case, I would add the tests with the prefixes being hardcoded in each env variable and then the main commit would port it to the new API. |
| if let Some(Some(ref prefix)) = a.env_prefix { | ||
| if let Some((ref env_name, _)) = a.env { | ||
| let prefixed = format!("{}_{}", prefix, env_name.to_str().unwrap_or("")); | ||
| let value = env::var_os(&prefixed); |
There was a problem hiding this comment.
Not thrilled with us having looked up the env and now we look it up again
There was a problem hiding this comment.
Yeah, this is a tradeoff with the current design. Arg::env() eagerly caches the env value at argument creation time, but we don't know the final env name until the prefix is applied during build. So we have to re-lookup with the prefixed name.
Moving the prefix application earlier (e.g. into arg_internal) doesn't help since Arg::env() already ran by then. The alternative would be lazy env lookup (only during parsing), but that'd be a bigger refactor of existing behavior. The extra env::var_os call at build time is cheap, and the old cached value gets replaced, so there's no functional issue - just a wasted initial lookup.
3450bca to
e8ca0e8
Compare
|
|
||
| #[cfg(feature = "string")] | ||
| #[test] | ||
| fn env_prefix_cli_overrides_env() { |
There was a problem hiding this comment.
This seems like it is testing env support generally rather than env-prefix-specific logic
There was a problem hiding this comment.
Good catch, removed that test. The remaining tests focus specifically on prefix behavior.
e8ca0e8 to
05b4713
Compare
There was a problem hiding this comment.
Please keep in mind that new tests should be added in the previous commit like the other new tests.
There was a problem hiding this comment.
Squashed everything into a single commit now - feature code, builder tests, and derive tests all together.
| #[test] | ||
| fn command_next_env_prefix_cli_overrides() { | ||
| env::set_var("DERIVE_CLI_NAME", "from_env"); | ||
|
|
||
| #[derive(Debug, Clone, Parser)] | ||
| #[command(next_env_prefix = "DERIVE_CLI")] | ||
| struct CliOptions { | ||
| #[arg(long, env = "NAME")] | ||
| name: Option<String>, | ||
| } | ||
|
|
||
| let m = CliOptions::try_parse_from(vec!["", "--name", "from_cli"]).unwrap(); | ||
| assert_eq!(m.name.as_deref(), Some("from_cli")); | ||
| } |
There was a problem hiding this comment.
This seems to be testing env support
There was a problem hiding this comment.
Removed that test - it was testing general env/cli override behavior, not prefix-specific logic.
| fn command_next_env_prefix_with_flatten() { | ||
| env::set_var("FLAT_APP_DB", "mydb"); | ||
|
|
||
| #[derive(Debug, Clone, Args)] | ||
| #[command(next_env_prefix = "FLAT_APP")] | ||
| struct DbArgs { | ||
| #[arg(long, env = "DB")] | ||
| db: Option<String>, | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, Parser)] | ||
| struct CliOptions { | ||
| #[command(flatten)] | ||
| db_args: DbArgs, | ||
| } |
There was a problem hiding this comment.
Missing some flatten cases that next_help_heading has, like
- flattened field with help heading
There was a problem hiding this comment.
Added more flatten cases:
flatten_multiple_structs_different_prefixes- two flattened structs each with their own prefix, verifying isolationparent_env_prefix_does_not_leak_into_flatten- parent command's prefix correctly doesn't leak into a flattened struct that doesn't set its own prefix
|
hey @epage, thanks for the thorough review — really helpful feedback. let me go through each point:
I'll rework the commits and push an updated version. appreciate the detailed review! |
05b4713 to
1a94daa
Compare
|
rebased and addressed the test feedback:
|
1a94daa to
46986d6
Compare
|
Thanks for the detailed review @epage. I see I'm missing several cases — the flatten scenarios that next_help_heading handles, derive tests, the getter, and the turbofish issue. Let me go through next_help_heading's implementation more carefully and make sure env_prefix covers the same set of cases. Will push an update with the missing flatten tests, a getter, and clean up the turbofish. |
|
fixed the CI failures — the env::set_var calls in my test code weren't wrapped in unsafe blocks, which is required in the 2024 edition. all set_var calls now have the unsafe wrapper matching the existing test style in the repo. |
Add env variable prefix support so that env variable names specified via Arg::env can be automatically prefixed during build. This works similarly to Command::next_help_heading - it's a stateful method that affects subsequently added arguments. - Command::next_env_prefix sets the prefix for all following args - Arg::env_prefix sets per-arg prefix (takes precedence) - Prefix and env name are joined with underscore - Hand-written Debug impl updated to include env_prefix - Getter get_env_prefix added for reflection - Derive support via #[command(next_env_prefix = "...")] - Works with flatten (prefix on Args struct or on flatten field) - Requires both `env` and `string` features since the prefixed env name is dynamically constructed Closes clap-rs#3221
5ebc02f to
d47433a
Compare
Adds
Command::next_env_prefixwhich sets a prefix that gets prepended to all subsequent arg env variable names during build. Modeled directly afternext_help_headingas @epage suggested in #3221.How it works:
Command::next_env_prefix("MYAPP")sets the prefix for all future argsenvname and an inherited prefix, the env name gets rewritten toMYAPP_<NAME>during_build_selfnext_env_prefix(None::<&str>)The env prefix is stored on each
Arg(viaenv_prefixfield) duringarg_internal, same pattern ashelp_heading. The actual name rewriting happens during_build_selfwhich keeps help generation and env lookup unchanged.Requires both
envandstringfeatures since we need runtime string construction for the prefixed names.Closes #3221