Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change misleading usage examples #37

Closed
xiaoas opened this issue Nov 2, 2022 · 2 comments · Fixed by #38
Closed

Change misleading usage examples #37

xiaoas opened this issue Nov 2, 2022 · 2 comments · Fixed by #38

Comments

@xiaoas
Copy link

xiaoas commented Nov 2, 2022

The example in README use dotenvy::dotenv feels miss-leading:

fn main() {
    dotenv().ok();

    for (key, value) in env::vars() {
        println!("{key}: {value}");
    }
}

calling .ok on dotenv(), a Result, turns it into a Option. This is not what most people might expect imo. Changing to some error handling might better serve the intention:

fn main() -> Result<(), dotenvy::Error>{
    dotenv()?; // or dotenv().unwrap();

    for (key, value) in env::vars() {
        println!("{key}: {value}");
    }
}
@sonro
Copy link
Collaborator

sonro commented Nov 2, 2022

The reason for the .ok() is to ignore the error.

The .env file this loads might not be available on the deployed version of a user's app. The environment variables would usually be set elsewhere. By using this pattern no error is thrown if the file isn't found.

As some users may require a .env file, they have the option of using its result type.


You are right: there needs to be better documentation around this issue. It certainly doesn't feel very "rusty" to just .ok() an error like that.

@allan2
Copy link
Owner

allan2 commented Nov 2, 2022

@xiaoas, thanks for reporting. It's been brought up a few times already that .ok() is not ideal.

I've replaced some .ok()s in the docs with .unwrap() but there are still a few instances left, including the README example.
Feel free to open a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants