with_env clearing the environment is jarring

[epage]

Quoting @sevagh:

Just to confirm that I expected the with_env(&vec[("MY_ENV_VAR", "MY_VAL")] to Just Work(TM) and it didn't - I had to use Environment::inherit().insert("MY_ENV_VAR", "MY_VAL").

Previously, I used to use https://doc.rust-lang.org/1.1.0/std/process/struct.Command.html#method.env - where:

let mut cmd = std::process::Command();
cmd.env("MY_ENV_VAR", "MY_VAL");

The usage there is just to append to the inherited environment variables, not to totally clear them.

I find the fact that with_env erases all the inherited env vars to be jarring. Any opinions?

[epage]

I agree that the choice might not be obvious. I can't find where in #46 we made the decision.

Some random thoughts on my part

• I think of a with_ as replacing the current value rather than augmenting it. If you specify a with_env(&[]) it feels like you are defining what you want the env to be and we shouldn't assume you want inherited variables in there
• Maybe a with_inherited_env if we want a short hand?
• Even if we make a shorthand, part of the problem is that we are using From<_> -> Environment for this which defaults to empty. Maybe with_inherited_env only takes a &[]?

[colin-kiegel]

What about with_env_append()? I'm not sure if it makes sense to call this repeatedly - but you could..

[epage]

When we put with in a function name, what is it trying to say?

After looking into how to implement a fix, I did have an idea for a name.

fn extend_env<I: IntoIterator<Item = (T, Z)>>(mut self, iter: I) -> self {
   self.env.extend(iter);
   self
}

See also https://github.com/Freyskeyd/environment/blob/master/src/lib.rs#L394

[epage]

A question to ask is how many of these helpers should we implement.

Unlike .stdout() and .stderr(), we chose for the client to pass env in rather than building the fluent API directly into the Assert. So we need to make sure we have a clear reason for why we are adding more parts of an env api rather than having people construct one explicitly and pass it in.

In this case, I think the problem is confusion over the API.

Documentation is one way to help this. I think why we don't have the docs is that we are combining multiple pieces here and the code assumes you are familiar with all of them when you might not be.

Proposal We update with_env documentation to add a note to the first example that the environment is not inherited. We should update the second example to use an inherited environment.

Sometimes offering an alternative route helps call out the behavior of the original one. So there might be value in creating a second function for this specific purpose and not as a convenience.

[sevagh]

I think - to be fair - that once the Cargo issue is resolved (which I believe was giving me the "[cargo binary] file not found" errors as discussed in #51), I wouldn't have noticed this as an issue as I typically don't rely on inherited env vars.

However the point stands that the std::process::Command.env() function appends and do you (we? I'm a contributor now :D) care about similarity of APIs with std::process?

[Freyskeyd]

@epage I like extend_env.

I feel comfortable on keeping with_env to override the whole environment (we need to mention it somewhere) and extend_env to extend the environment with user variables.

[epage]

So I feel like we have a good idea on how we plan to handle this

• Update docs
• Add extend_env

Moving this from question to bug (the bug is the API confusion, not an implementation bug)

[Freyskeyd]

@epage are you working on fixing this bug?

[epage]

Not yet but if someone needs me to prioritize this I can.