-
-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
api(Arg): add from_env #1057
api(Arg): add from_env #1057
Conversation
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.
Some quick notes about the changes and questions. Let me know if anything needs to be significantly changed.
src/app/parser.rs
Outdated
@@ -1790,6 +1790,38 @@ impl<'a, 'b> Parser<'a, 'b> | |||
} | |||
Ok(()) | |||
} | |||
|
|||
pub fn add_from_env(&mut self, matcher: &mut ArgMatcher<'a>) -> ClapResult<()> { | |||
macro_rules! add_val { |
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.
This is duplicated from add_defaults
, I'll make sure to dedup before final submission.
src/app/validator.rs
Outdated
@@ -28,6 +28,7 @@ impl<'a, 'b, 'z> Validator<'a, 'b, 'z> { | |||
-> ClapResult<()> { | |||
debugln!("Validator::validate;"); | |||
let mut reqs_validated = false; | |||
self.0.add_from_env(matcher)?; |
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.
Need to validate that this maintains the ordering in tests.
@@ -11,19 +11,25 @@ use vec_map::{self, VecMap}; | |||
|
|||
// Internal | |||
use Arg; | |||
use args::{ArgSettings, Base, Switched, AnyArg, DispOrder}; | |||
use args::{AnyArg, ArgSettings, Base, DispOrder, Switched}; |
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.
let me know if you don't want the default rustfmt changes to be merged in... this is current rustfmt, at least up to a few days ago.
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.
I'm good with the rustfmt changes. I try to stay using the latest and greatest update so as to stick with community best practices.
@@ -8,8 +8,9 @@ use clap::{App, Arg, ErrorKind}; | |||
#[test] | |||
fn opts() { | |||
let r = App::new("df") | |||
.arg( Arg::from_usage("-o [opt] 'some opt'") | |||
.default_value("default")) | |||
.arg( |
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.
lots of rustfmt changes...
tests/default_vals.rs
Outdated
.get_matches_from_safe(vec!["test", "--input", "some", "--output", "other"]); | ||
|
||
assert!(m.is_ok()); | ||
let m = m.unwrap(); | ||
assert_eq!(m.value_of("output"), Some("other")); | ||
assert_eq!(m.value_of("input"), Some("some")); | ||
} | ||
|
||
#[test] |
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.
additional test I'll be adding: validate order when default is present and no arg. let me know if there are others you think are needed...
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.
My biggest concern for testing is to ensure correct order (from highest to lowest):
- Args from command line
- ENV vars
- Default Values
Also a test to confirm something along the lines of:
$ MYVAR='some' program --option
Where --option
accepts 0 or one value. In this case, matches.value_of("option") == None
because the empty value was specified at the command line (same for --option=
and --option ""
).
The only one I'm a little unsure about is:
$ MYVAR='some' program --option
Where --option
accepts 0 or more values (i.e. multiples). Should the result be ["", "some"]
for the values, or None
? The same goes for $ MYVAR='some' program --option=other
. Should it be ["other", "some"]
or ["other"]
?
I'm actually fine with either way, so long as it's documented which way it'll happen. I'm inclined to lean specified args (at the command line) override env vars entirely, so the examples above would result in None
and ["other"]
respectively. Otherwise there is no way to "override" ENV vars without manually resetting them $ MYVAR="" program --option
which would be a pain.
The case for "merging" env vars and command line options would be more in line with allowing external argv's as in $ MYARGV="--option some" program --option other
. But that's a whole other issue 😜 so let's not worry about it here.
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.
I'd be concerned with merging as opposed to overriding. I suppose we could allow that through some other mechanism. But I think it would get confusing as to how to maintain that from a user perspective.
As to multi-opts, I'll throw in as many test cases as I can and we'll see how far we get...
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.
Yeah I don't want to mess with merging right now. I'd like to stick with just overriding 😄
src/app/help.rs
Outdated
@@ -474,6 +474,7 @@ impl<'a> Help<'a> { | |||
Format::None(pv.to_string_lossy()) | |||
})); | |||
} | |||
// FIXME: add [env::{}: {}] here |
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.
to do this, I'll need to keep a reference to the original OsStr key for the env.
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.
Since it's meant for display, I'm fine with using to_string_lossy
to create a displayable string. If it becomes an issue, we can always change it to an OsStr and display the actual bytes later in a non-breaking way.
src/args/arg.rs
Outdated
pub fn from_env<K: AsRef<OsStr>>(mut self, name: K) -> Self { | ||
self.setb(ArgSettings::TakesValue); | ||
|
||
self.v.from_env = env::var_os(name); |
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.
this needs to be a tuple of OsStr and the env::var, I'll be adding that in a subsequent change.
@@ -3311,6 +3312,16 @@ impl<'a, 'b> Arg<'a, 'b> { | |||
self | |||
} | |||
|
|||
/// Specifies that if the value is not passed in as an argument, that it should be retrieved |
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.
I'll try and add similar docs to the others.
src/args/arg.rs
Outdated
/// Specifies that if the value is not passed in as an argument, that it should be retrieved | ||
/// from the environment if available. If it is not present in the environment, then default | ||
/// rules will apply. | ||
pub fn from_env<K: AsRef<OsStr>>(mut self, name: K) -> Self { |
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.
It's nitpicky (or rather, bikeshedable), but I'd rather use a name such as simply env
or env_var
so as not to confuse anyone. Typically, from_*
are constructors which create the Arg
from something, in a similar manner to From<T>
traits.
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.
yeah, I was thinking about that. The linked issue has env_fallback
. Personally I'm fine with just env
Looks good so far 👍 I've made some comments on the changes and am looking forward to merging this 🎉 |
src/args/arg.rs
Outdated
@@ -3315,10 +3315,10 @@ impl<'a, 'b> Arg<'a, 'b> { | |||
/// Specifies that if the value is not passed in as an argument, that it should be retrieved | |||
/// from the environment if available. If it is not present in the environment, then default | |||
/// rules will apply. | |||
pub fn from_env<K: AsRef<OsStr>>(mut self, name: K) -> Self { | |||
pub fn env(mut self, name: &'a OsStr) -> Self { |
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.
I think by convention, this should be env_os
and there would also be another method env
which would accept an &str
operating with env::var
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.
I notice in most of the other code, it's using OsStr::from_bytes, rather than OsStr::new(..), any particular reason?
There are a few more tests I need to add, for things like validation, etc. |
pub fn add_env(&mut self, matcher: &mut ArgMatcher<'a>) -> ClapResult<()> { | ||
macro_rules! add_val { | ||
($_self:ident, $a:ident, $m:ident) => { | ||
if let Some(ref val) = $a.v.env { |
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.
I ended up not trying to dedup this with default. I found it was going to be annoying. If it's required for commit, I can, but otherwise I'd prefer to leave as is.
Ok, I think this is ready. Let me know if you want me to rebase any of the commits. |
It's not obvious to me why that job is failing. |
@kbknapp I'm all done with my changes on this. Let me know if you want to see anything else. Thanks! |
Woohoo! 🎉 Thanks for tackling this issue! I'll fix the merge conflict and merge 😉 |
Closing this one, will merge #1062 |
This is not ready for merge. I want to add the help strings and additional tests. But I wanted to get some feedback.
This is for #814
This change is