allan2 / dotenvy

A well-maintained fork of the dotenv crate
MIT License
664 stars 40 forks source link

Improve error handling by not considering "not found" an error #44

Closed kaj closed 1 year ago

kaj commented 1 year ago

Currently, the examles and documentation generally ignores error from dotenvy.

    let _ = dotenvy::dotenv(); // either like this
    dotenvy::dotenv().ok(); // or like this

This is generally bad practice, and makes debugging of syntax errors in .env files harder than it needs to be. The general if is of course to use either .unwrap() or ? in all documentation examples. But there is a problem with this; checking for errors makes it obvious that there actually are errors when there should not be, since the dotenv() function returns an error if there is not .env file.

I think having a syntax error in the .env file is a problem I want to know if I have, and not beeing allowed to read my .env file is a problem I want to know if I have. But I don't have a .env file in production (where I insert the environment by other means), so the dotenv() call must not terminate my program in that case.

Currently, I use a simple wrapper:

fn dotenv() -> dotenvy::Result<()> {
    match dotenvy::dotenv() {
        Ok(_) => Ok(()),
        Err(e) if e.not_found() => Ok(()),
        Err(e) => Err(e),
    }
}

But it would be nicer to fix the dotenvy::dotenv() method directly. It currently returns Result<PathBuf >. I agree that getting the used path (if any) can be useful, so I think it should return Result<Option<PathBuf>>, where a successfully parsed file is signified by Ok(Some(path)), and not having a .env file is signified by Ok(None), and only actual failure to read or parse an existing file yields an Err(...).

If you agree to this, I'd be willing to write a pull request for it.

kaj commented 1 year ago

Addendum: If there is an error, I think that error should also include the path of the attempted file.

sonro commented 1 year ago

Thank you for your detailed examination. We are discussing this issue here in #39.

I like your solution to use a Result<Option<PathBuf>> as the return type. However, as you'll see in the discussion (and in some issues/PRs), there are subsections of users who consider the lack of a .env file to be an error. I'd be very interested in your thoughts on providing for both use-cases. Currently, we are moving towards providing more extensive documentation to show users how to better handle/ignore errors.

kaj commented 1 year ago

Oh, I did some searching, but aparently for the wrong keywords, so I missed #39 . Sorry.