Early support for reading program arguments from env-variable (issue #712) #734

purew · 2016-11-06T18:20:59Z

Hi Kevin,

This is Anders from Thursday's meetup. Take a look at the code and let me know what you want changed or what is missing?

Also, I wasn't sure where you put the unit-tests so it went in the same file. I also didn't spend any time on documentation until the interface is ironed out.

This change is

coveralls · 2016-11-06T18:41:31Z

Coverage increased (+0.009%) to 91.264% when pulling df90d4a on PureW:master into 087cee7 on kbknapp:master.

kbknapp

Looks great so far, I think if we make those two simple changes I'd be good with a merge.

kbknapp · 2016-11-06T18:52:43Z

src/app/mod.rs

@@ -30,6 +31,8 @@ use errors::Result as ClapResult;
 pub use self::settings::AppSettings;
 use completions::Shell;

+const DEFAULT_ARGS_ENV_VAR: &'static str = "VAR";


I think we can do away with a default variable name. The user typing None is just as if they'd typed the env var name VAR

kbknapp · 2016-11-06T18:56:07Z

src/app/mod.rs

+    /// env-var `env_var_name`.
+    ///
+    /// Defaults to read from "VAR" if `env_var_name` is `None`.
+    pub fn get_matches_with_env(self, env_var_name: Option<String>)


Let's change this function declaration to:

pub fn get_matches_with_env<S: Into<&'b str>(self, env_var_name: S) -> ArgMatches<'a> {

This will allow people to use string slices too and not have to allocate a string if they don't need to. This will also require a few lines to change inside the function too, but not many.

Ahh right, I've used the Into-trait once or twice before. I always have to lookup the syntax though :)

Shouldn't that be <S:AsRef<str>(self, env_var_name: &S)?
env_var_name isn't stored anywhere, so why the lifetime 'b?
And &str doesn't implement From<&String>.

@tormol Good catch! Yeah the AsRef trait should be used, not Into

kbknapp · 2016-11-06T19:02:26Z

Also, when you change these commits, could you use the commit subject to something like feat: adds the ability to get matches from environmental vars?

This will allow that commit to be picked up automatically by clog and put into the next versions change log. 😄

kbknapp · 2016-11-06T19:03:30Z

Ah yeah, and the unit tests are fine where they are. I'm good with using those or placing them as integration tests in the tests/ directory. Thanks again for tackling this issue! 👍

purew · 2016-11-06T19:10:16Z

About the commit-message, should I rebase the history to a single commit?

EDIT: Nevermind, I found the contribution-guidelines!

purew · 2016-11-06T19:36:20Z

I just realized that this simple solution might be too simple. Take the following case:

VAR="--arg1 val1" ./prog --arg1 val2 --arg1 val3

if the program calls values_of("arg1") I think it returns (val1, val2, val3) which is likely not what the user wants. The program arguments should take complete precedence over env-var I believe...

tormol · 2016-11-06T20:15:55Z

src/app/mod.rs

+                       .chain(vargs.split(0x20)
+                                   .into_iter()
+                                   .map(|s| s.to_os_string()))
+                       .collect();


You can use b' ' instead of 0x20
.split() returns an iterator (osstringext::OsSplit), so .into_iter() does nothing.
Vec implements std::iter::Extend so this can be written as args.extend(vargs.split(b' ').map(...)).

tormol · 2016-11-06T21:10:49Z

.chain() will return the elements of the first iterator first, so you would get ["arg2,"arg3","arg1"].
Putting env first will allow options in the program arguments to override those set by ARG.

There are some other issues with concatenating:

ARG='--' ./prog --opt will turn --opt into an argument
ARG='--opt-with-optional-argument' ./prog arg will make arg an argument to the option.

Fixing that might be difficult, and i'd guess environmental variables will mostly be used to set options, so I don't think this is a big problem.

Separating arguments with .split() makes it impossible to give arguments with spaces in them,
and repeated spaces results in n-1 empty slices.

bash doesn't seem to split on non-ASCII whitespace either, so that's probably fine but you might want to separate on tabs and newlines too.

kbknapp · 2016-11-06T21:12:14Z

I just realized that this simple solution might be too simple...

Ah yeah, I'd forgotten we talked about that. There's not really a good way around this, unless we keep track of how a particular argument was matched. That may be quite a bit more complicated to implement.

kbknapp · 2016-11-06T21:18:33Z

@tormol regards to your bullet points above, I think so long as the env args are always first, or last it's fine. So long as it's deterministic and documented that way.

Personally I'd prefer that the env args come first. Allowing them to be overriden if the consumer of the library uses Arg::overrides_with. Either way though, we'll need some way to solve the $ VAR="--opt val1" prog --opt val2. I'm of the mind that env args are of a lower precedent than manually input args.

There are quite a few edge cases that have many different correct answers depending on use case.

If an option allows multiple values, should $ VAR="--opt val1" prog --opt val2 cause the final value to be val2, or val1, val2? Etc.

kbknapp · 2016-11-06T21:20:45Z

We could also have different methods for _pre_pending vs _ap_pending these env args.

Refs clap-rs#712.

purew · 2016-11-10T04:09:19Z

Just pushed a rebased commit, but I'm having a problem with an error I haven't seen before:

error[E0282]: unable to infer enough type information about `_`
    --> src/app/mod.rs:1655:44
     |
1655 |                                           .get_matches_with_env(None);
     |                                            ^^^^^^^^^^^^^^^^^^^^ cannot infer type for `_`
     |
     = note: type annotations or generic parameter binding required

Doesn't sound like a difficult error but I don't understand what _ is...

I tried splitting on space, tab, and newline but OsStr.split does seems to only support splitting on a char, see comments.

kbknapp · 2016-11-10T16:02:13Z

The error comes from rustc not being able to tell what kind of Option<T> to use for the None value. I forget the exact syntax, but you'd have to do something like Option::<OsStr>::None This is part of the reason I'd rather just remove the Option and default VAR.

tormol · 2016-11-10T23:29:22Z

I have written an iterator that returns the arguments in a variable.
It supports escaping bytes with \ and quoting arguments with " and '.
It's completely untested.

purew · 2016-11-11T13:53:56Z

This is part of the reason I'd rather just remove the Option and default VAR

@kbknapp Ohh, I misunderstood your first comment about VAR. Just pushed commit that removes the Option.

@tormol Looks interesting! Is it safe to put byte-sequences in env-vars?

coveralls · 2016-11-11T14:14:10Z

Coverage increased (+1.4%) to 92.674% when pulling c20a61c on PureW:master into 087cee7 on kbknapp:master.

kbknapp · 2016-11-11T19:13:45Z

src/app/mod.rs

+    /// Similar to [`App::get_matches`] but also reads args from
+    /// env-var `env_var_name`.
+    ///
+    /// If the same argument is specified both as program argument


Let's change this to say, "The env var args are appended to the manually entered arguments (if any) and thus if an argument is provided both in the env var and manually, this could result in either a conflict, override, or additional value depending on the specific rules for said arg."

kbknapp · 2016-11-11T19:17:00Z

@purew looks great! Could you just change that one comment so that it's more explicit? This way people know no magic is happening, and they must be careful.

Once that is done, @tormol if you'd like to give any other advice I think this will be good for a merge.

Once this is merged I'd also be interested in adding in the splitting that @tormol wrote once we can add some tests which could be done as a separate PR.

coveralls · 2016-11-13T22:15:50Z

Coverage decreased (-0.07%) to 91.181% when pulling b9ea3bf on PureW:master into 087cee7 on kbknapp:master.

tormol · 2016-11-13T22:23:53Z

@purew: safe/unsafe for who?
I wrote escape bytes because i got uncertain, but I don't see any problems with multibyte/non-ASCII characters.

purew · 2016-11-13T22:28:57Z

@tormol I was merely curious about byte-sequences in env-vars and if common shell's allow that?

tormol · 2016-11-15T00:42:41Z

VAR=`echo -e '\xff'` works in bash.

kbknapp · 2016-11-15T22:08:35Z

@purew see #748 where @casey mentions having a MergeType enum which specifies how conflicts between these various "other ways" to specify arguments (command line, env, and files). I haven't decided on exactly how I want to go about that, and what sort of naming I want to use (which is kind of a 🚲 🏚️ ), but if I decide to use something like that, I'd want it to apply to this PR as well, and can't add that in after the fact. So I'm going to put this on hold for a short time until I solidify how I want that API to look. I'll post back here once I've got the details 😉

homu · 2016-12-28T23:43:28Z

☔ The latest upstream changes (presumably #784) made this pull request unmergeable. Please resolve the merge conflicts.

kbknapp · 2017-05-16T02:21:17Z

I'm going to close this due to inactivity and punting on this issue until I have a little more free-time to work on it - feel free to re-open and we can re-energize it.

purew · 2017-05-16T13:00:13Z

Ok, are all "upstream" blockers resolved? You mentioned solidifying a particular API before continuing on this PR:

So I'm going to put this on hold for a short time until I solidify how I want that API to look. I'll post back here once I've got the details

kbknapp · 2017-05-16T13:13:17Z

@purew for single args getting a value from the ENV all blockers are solved. That could be merged today. The precedence is CLI overrides all, ENV overrides default vals, and defaults override empty values. The Arg API could be Arg::env_var.

The allowing an ENV var to hold an arbitrary number of args (such as VAR='some --opt=val) I still haven't even decided on the specifics. It will be something along the lines of allowing an "external argv" but the merge semantics haven't been decided yet. This is mostly due to my not having had much time this year to work on side projects, which is unfortunate.

kbknapp requested changes Nov 6, 2016

View reviewed changes

tormol reviewed Nov 6, 2016

View reviewed changes

feat: adds the ability to get matches from environmental vars

961a026

Refs clap-rs#712.

purew force-pushed the master branch from df90d4a to 961a026 Compare November 10, 2016 04:05

Remove Option in argument to get_matches_with_env

c20a61c

kbknapp reviewed Nov 11, 2016

View reviewed changes

kbknapp added the R: work in progress label Nov 11, 2016

Clearer doc-string

b9ea3bf

kbknapp closed this May 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Early support for reading program arguments from env-variable (issue #712) #734

Early support for reading program arguments from env-variable (issue #712) #734

purew commented Nov 6, 2016 •

edited by kbknapp

Loading

coveralls commented Nov 6, 2016 •

edited

Loading

kbknapp left a comment

kbknapp Nov 6, 2016

kbknapp Nov 6, 2016

purew Nov 6, 2016

tormol Nov 6, 2016

kbknapp Nov 6, 2016

kbknapp commented Nov 6, 2016

kbknapp commented Nov 6, 2016

purew commented Nov 6, 2016 •

edited

Loading

purew commented Nov 6, 2016

tormol Nov 6, 2016

tormol commented Nov 6, 2016

kbknapp commented Nov 6, 2016

kbknapp commented Nov 6, 2016

kbknapp commented Nov 6, 2016

purew commented Nov 10, 2016

kbknapp commented Nov 10, 2016

tormol commented Nov 10, 2016

purew commented Nov 11, 2016

coveralls commented Nov 11, 2016 •

edited

Loading

kbknapp Nov 11, 2016

kbknapp commented Nov 11, 2016

coveralls commented Nov 13, 2016 •

edited

Loading

tormol commented Nov 13, 2016

purew commented Nov 13, 2016

tormol commented Nov 15, 2016 •

edited

Loading

kbknapp commented Nov 15, 2016

homu commented Dec 28, 2016

kbknapp commented May 16, 2017

purew commented May 16, 2017

kbknapp commented May 16, 2017

Early support for reading program arguments from env-variable (issue #712) #734

Early support for reading program arguments from env-variable (issue #712) #734

Conversation

purew commented Nov 6, 2016 • edited by kbknapp Loading

coveralls commented Nov 6, 2016 • edited Loading

kbknapp left a comment

Choose a reason for hiding this comment

kbknapp Nov 6, 2016

Choose a reason for hiding this comment

kbknapp Nov 6, 2016

Choose a reason for hiding this comment

purew Nov 6, 2016

Choose a reason for hiding this comment

tormol Nov 6, 2016

Choose a reason for hiding this comment

kbknapp Nov 6, 2016

Choose a reason for hiding this comment

kbknapp commented Nov 6, 2016

kbknapp commented Nov 6, 2016

purew commented Nov 6, 2016 • edited Loading

purew commented Nov 6, 2016

tormol Nov 6, 2016

Choose a reason for hiding this comment

tormol commented Nov 6, 2016

kbknapp commented Nov 6, 2016

kbknapp commented Nov 6, 2016

kbknapp commented Nov 6, 2016

purew commented Nov 10, 2016

kbknapp commented Nov 10, 2016

tormol commented Nov 10, 2016

purew commented Nov 11, 2016

coveralls commented Nov 11, 2016 • edited Loading

kbknapp Nov 11, 2016

Choose a reason for hiding this comment

kbknapp commented Nov 11, 2016

coveralls commented Nov 13, 2016 • edited Loading

tormol commented Nov 13, 2016

purew commented Nov 13, 2016

tormol commented Nov 15, 2016 • edited Loading

kbknapp commented Nov 15, 2016

homu commented Dec 28, 2016

kbknapp commented May 16, 2017

purew commented May 16, 2017

kbknapp commented May 16, 2017

purew commented Nov 6, 2016 •

edited by kbknapp

Loading

coveralls commented Nov 6, 2016 •

edited

Loading

purew commented Nov 6, 2016 •

edited

Loading

coveralls commented Nov 11, 2016 •

edited

Loading

coveralls commented Nov 13, 2016 •

edited

Loading

tormol commented Nov 15, 2016 •

edited

Loading