Open abonander opened 4 years ago
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);
}
}
}
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.
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.
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-283Namely, 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
orenvironment 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 thinkdotenv::var()
should definitely return a more useful error here, which gives it additional utility as well.