dotenv-rs / dotenv

Library to help supply environment variables in testing and development
MIT License
554 stars 84 forks source link

What is the reason of deprecaiting iter methods? #13

Open mvlabat opened 5 years ago

mvlabat commented 5 years ago

Greetings! Thank you for continuing to maintain this project and creating the repository.

I updated my project to the new version dotenv and saw that iter methods were deprecated. As for me, I found them quite useful in my project, because they allowed me to read dotenv files, preprocess variables and then to decide which of them and how I store in the environment. With deprecating those my only option is to iterate though vars() (which is basically just env::var()) and I don't have any way to detect whether those come from the system or from the dotenv file.

Could we consider un-deprecating those methods? Thank you

ZoeyR commented 5 years ago

The reason was that all the cases I could find of people using the iter methods was to use the dotenv crate for config files instead of actually merging them with the environment. dotenv is not intended for managing config files per-se, it's for managing environment variables. If you have a legitimate usecase for inspecting the variables before sending them into the environment I'd love to hear it! If there are compelling reasons to do the inspection I will consider un-deprecating these methods.

alex35mil commented 5 years ago

Not sure if it's a valid usecase, but: I manage my infra via rust binary and this binary reads env files and runs various of tasks. One of the examples is it starts development environment + specs in watch mode. Running processes use different env files. For such case, I prefer not to load anything in parent environment but read appropriate file and pipe result to environment of appropriate child process. TBH I'm not sure if this change might actually cause issues but global state makes me worry by default and I would prefer not to have it when it's not required and potentially might cause problems.

alex35mil commented 5 years ago

I think I can imagine issues here. E.g. if I forgot to pipe env vars to one of the child, with clean parent environment it would crash due to absence of the data. But when parent environment is populated with the wrong set of vars, then it would inherit it and hello debugging fun.

ZoeyR commented 5 years ago

@alexfedoseev just to make sure I understand, you launch a child process and then modify its environment with the contents of a .env file? If this is the case, why doesn't the child process run the .env file itself?

alex35mil commented 5 years ago

@dgriffen I abstracted it away so process uses provided env vars, e.g.:

pub struct Cmd {
  pub bin: &'static str,
  pub args: Args,
  pub env: EnvVars,
  pub cd: Dir,
  pub msg: &'static str,
}

pub fn up() -> Cmd {
  Cmd {
    bin: "cargo",
    args: Args::new(vec!["watch", "-x", "run"]),
    env: DEV_ENV.all(),
    cd: Dir::CoreServer,
    msg: "Running reloadable Core development server",
  }
}

Then returned Cmd is used by the executor. This allows me to use same abstraction for processes that need all vars from the .env file as well as processes that need only few (or don't need env at all).

albel727 commented 5 years ago

If this is the case, why doesn't the child process run the .env file itself? I can answer this question for my use case. Because dotenv::dotenv() doesn't actually override already set environment variables. This is apparently by design, but I actually want the opposite. I'm not sure why this is not an option, but I have to use dotenv_iter() hack like

Command::new("env").envs(dotenv::dotenv_iter()?.collect::<Result<Vec<_>,_>>()?).status()?;

in order to achieve what I want at least for subprocesses.

mvlabat commented 5 years ago

Hey @dgriffen! Sorry it took me so long to answer.. In my project in order to set environment variables for the application I require to use APP_NAME_ prefix for all the variables keys. At the same time in .env I allow users to omit that prefix, because people can be confident that the variables are applied exactly to the application. This way I can reduce some boilerplate, especially if the prefix is quite long.

So the thing I do with preprocessing is prepending APP_NAME_ prefix to every key that is specified in .env (if the prefix is not used already).

With deprecating iter methods I'll loose that convenience and I'll have to either bring prefix boilerplate back to .env or to resort to some hacks, that I'd really prefer to avoid, like iterating through a whole list of environment variables and mutating a subset of them.

ZoeyR commented 5 years ago

@mvlabat okay, I can definitely see that as a use-case! I'll go ahead and undeprecate thse methods in 0.14.2. I will probably change their return type signature slightly though since I don't like that they expose an internal iterator type.

alex35mil commented 4 years ago

It doesn't seem like those methods were undepricated. Even though it's mentioned in CHANGELOG.

ZoeyR commented 4 years ago

@alexfedoseev thanks for reporting, I forgot to actually undeprecate them.

Plecra commented 4 years ago

That PR has all but wrapped this up. I'm pretty sure this issue can be closed