api(Arg): add from_env #1057

bluejekyll · 2017-09-29T19:01:54Z

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

coveralls · 2017-09-29T19:49:26Z

Coverage decreased (-0.05%) to 93.624% when pulling 306807f on bluejekyll:from_env into 6e94899 on kbknapp:master.

bluejekyll

Some quick notes about the changes and questions. Let me know if anything needs to be significantly changed.

bluejekyll · 2017-09-30T00:18:53Z

src/app/parser.rs

@@ -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 {


This is duplicated from add_defaults, I'll make sure to dedup before final submission.

bluejekyll · 2017-09-30T00:19:21Z

src/app/validator.rs

@@ -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)?;


Need to validate that this maintains the ordering in tests.

bluejekyll · 2017-09-30T00:20:39Z

src/args/arg_builder/flag.rs

@@ -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};


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.

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.

bluejekyll · 2017-09-30T00:22:05Z

tests/default_vals.rs

@@ -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(


lots of rustfmt changes...

bluejekyll · 2017-09-30T00:23:33Z

tests/default_vals.rs

        .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]


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...

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.

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...

Yeah I don't want to mess with merging right now. I'd like to stick with just overriding 😄

bluejekyll · 2017-09-30T00:25:04Z

src/app/help.rs

@@ -474,6 +474,7 @@ impl<'a> Help<'a> {
                                           Format::None(pv.to_string_lossy())
                                       }));
            }
+            // FIXME: add [env::{}: {}] here


to do this, I'll need to keep a reference to the original OsStr key for the env.

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.

bluejekyll · 2017-09-30T00:26:37Z

src/args/arg.rs

+    pub fn from_env<K: AsRef<OsStr>>(mut self, name: K) -> Self {
+        self.setb(ArgSettings::TakesValue);
+
+        self.v.from_env = env::var_os(name);


this needs to be a tuple of OsStr and the env::var, I'll be adding that in a subsequent change.

bluejekyll · 2017-09-30T00:26:40Z

src/args/arg.rs

@@ -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


I'll try and add similar docs to the others.

kbknapp · 2017-09-30T17:25:10Z

src/args/arg.rs

+    /// 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 {


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.

yeah, I was thinking about that. The linked issue has env_fallback. Personally I'm fine with just env

kbknapp · 2017-09-30T17:26:00Z

Looks good so far 👍 I've made some comments on the changes and am looking forward to merging this 🎉

kbknapp · 2017-10-04T01:53:32Z

src/args/arg.rs

@@ -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 {


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

I notice in most of the other code, it's using OsStr::from_bytes, rather than OsStr::new(..), any particular reason?

bluejekyll · 2017-10-05T17:43:39Z

There are a few more tests I need to add, for things like validation, etc.

coveralls · 2017-10-05T18:40:57Z

Coverage increased (+0.1%) to 93.786% when pulling 5fccd1f on bluejekyll:from_env into 3224e2e on kbknapp:master.

bluejekyll · 2017-10-05T18:55:17Z

src/app/parser.rs

+    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 {


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.

bluejekyll · 2017-10-05T18:55:48Z

Ok, I think this is ready. Let me know if you want me to rebase any of the commits.

coveralls · 2017-10-05T19:49:02Z

Coverage increased (+0.2%) to 93.869% when pulling 1401faa on bluejekyll:from_env into 3224e2e on kbknapp:master.

bluejekyll · 2017-10-05T20:37:49Z

It's not obvious to me why that job is failing.

coveralls · 2017-10-05T21:34:52Z

Coverage increased (+0.2%) to 93.869% when pulling 81cae1f on bluejekyll:from_env into 3224e2e on kbknapp:master.

coveralls · 2017-10-05T23:01:01Z

Coverage increased (+0.2%) to 93.907% when pulling 2fa8f83 on bluejekyll:from_env into 3224e2e on kbknapp:master.

bluejekyll · 2017-10-06T17:23:12Z

@kbknapp I'm all done with my changes on this. Let me know if you want to see anything else.

Thanks!

kbknapp · 2017-10-07T17:08:57Z

Woohoo! 🎉 Thanks for tackling this issue! I'll fix the merge conflict and merge 😉

kbknapp · 2017-10-07T17:31:02Z

Closing this one, will merge #1062

bluejekyll commented Sep 30, 2017

View reviewed changes

kbknapp added the R: work in progress label Sep 30, 2017

kbknapp reviewed Sep 30, 2017

View reviewed changes

kbknapp mentioned this pull request Sep 30, 2017

Values from environment variables #814

Closed

kbknapp reviewed Oct 4, 2017

View reviewed changes

benj-fry-sf added 5 commits October 5, 2017 10:46

WIP: add from_env option

f254807

add env key for help msg

bad5d19

add documentation and examples

ec46b55

fix some typos

779560b

add additional tests

5fccd1f

bluejekyll force-pushed the from_env branch from 58da290 to 5fccd1f Compare October 5, 2017 17:50

add validation tests

1401faa

bluejekyll changed the title ~~wip(Arg): add from_env~~ api(Arg): add from_env Oct 5, 2017

bluejekyll commented Oct 5, 2017

View reviewed changes

add space in help output

81cae1f

put env: before default as that will be chosen first

2fa8f83

kbknapp closed this Oct 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api(Arg): add from_env #1057

api(Arg): add from_env #1057

bluejekyll commented Sep 29, 2017 •

edited

Loading

coveralls commented Sep 29, 2017

bluejekyll left a comment

bluejekyll Sep 30, 2017

bluejekyll Sep 30, 2017

bluejekyll Sep 30, 2017

kbknapp Sep 30, 2017

bluejekyll Sep 30, 2017

bluejekyll Sep 30, 2017

kbknapp Sep 30, 2017

bluejekyll Sep 30, 2017 •

edited

Loading

kbknapp Oct 2, 2017

bluejekyll Sep 30, 2017

kbknapp Sep 30, 2017

bluejekyll Sep 30, 2017

bluejekyll Sep 30, 2017

kbknapp Sep 30, 2017 •

edited

Loading

bluejekyll Sep 30, 2017

kbknapp commented Sep 30, 2017

kbknapp Oct 4, 2017 •

edited

Loading

bluejekyll Oct 4, 2017

bluejekyll commented Oct 5, 2017

coveralls commented Oct 5, 2017

bluejekyll Oct 5, 2017

bluejekyll commented Oct 5, 2017 •

edited

Loading

coveralls commented Oct 5, 2017

bluejekyll commented Oct 5, 2017

coveralls commented Oct 5, 2017

coveralls commented Oct 5, 2017

bluejekyll commented Oct 6, 2017

kbknapp commented Oct 7, 2017

kbknapp commented Oct 7, 2017

api(Arg): add from_env #1057

api(Arg): add from_env #1057

Conversation

bluejekyll commented Sep 29, 2017 • edited Loading

coveralls commented Sep 29, 2017

bluejekyll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bluejekyll Sep 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbknapp Sep 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbknapp commented Sep 30, 2017

kbknapp Oct 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bluejekyll commented Oct 5, 2017

coveralls commented Oct 5, 2017

Choose a reason for hiding this comment

bluejekyll commented Oct 5, 2017 • edited Loading

coveralls commented Oct 5, 2017

bluejekyll commented Oct 5, 2017

coveralls commented Oct 5, 2017

coveralls commented Oct 5, 2017

bluejekyll commented Oct 6, 2017

kbknapp commented Oct 7, 2017

kbknapp commented Oct 7, 2017

bluejekyll commented Sep 29, 2017 •

edited

Loading

bluejekyll Sep 30, 2017 •

edited

Loading

kbknapp Sep 30, 2017 •

edited

Loading

kbknapp Oct 4, 2017 •

edited

Loading

bluejekyll commented Oct 5, 2017 •

edited

Loading