dotenv::var()'s error should report the variable that caused it

[abonander]

Currently this forwards the underlying std::env::VarError which has very unhelpful error messages: https://doc.rust-lang.org/src/std/env.rs.html#274-283

Namely, it doesn't report the variable that caused the error, which is useless for almost all intents and purposes unless you only ever invoke exactly it once in your application; even then, if someone tries to use your application that isn't familiar with this, they just get environment variable not found or environment variable was not valid unicode: <generic Utf8Error message>.

Unfortunately, this can't be changed in the stdlib for backcompat purposes since VarError is an enum, but I think dotenv::var() should definitely return a more useful error here, which gives it additional utility as well.

[Plecra]

It's generally considered bad practice to include the arguments in your error type. Users want to be able to return errors about local values, and don't want to make redundant allocations.

User facing errors, on the other hand, do need this context. Your UI can include logic to report missing variables itself:

pub fn var<K: AsRef<OsStr>>(key: K) -> String {
    let s = key.as_ref();
    match env::var(&s) {
        Ok(s) => s,
        Err(e) => {
            eprintln!("Couldn't get {}: {}", s.to_string_lossy(), e);
            std::process::exit(1);
        }
    }
}

[abonander]

async-std reports context in its I/O errors and this was generally considered a very nice UX improvement: https://docs.rs/async-std/1.6.2/src/async_std/fs/file.rs.html#117

We do try to wrap calls to env::var() where possible to add this context but it's difficult to enforce. There's no lint that I know of to warn people when they should be using a custom-defined function rather than a library-defined function.

Eliminating allocations where possible is a noble effort but we have to consider the context of where this function is usually called. It's never really called in hot loops or more than a handful of times in an entire application. Does it really matter if it allocates or not? If the user ignores the error value I'd be willing to bet the allocation is elided entirely in release mode.

[marcospb19]

Alocations should be avoided if they are redundant, but in this case, I think they are justified.

crate::Error not reporting the variable that caused the error forces some of us to create our custom implementation, which limits the usefulness of this crate.

I believe that errors should be able to have costful operations if it improves debugging for the developer or final user, because these errors usually occur only once and the program exits.

[allan2]

I've been thinking about how to make errors more helpful in dotenvy.

There's an issue here and a discussion here. Feedback and suggestions are welcome!