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

fix(docs): Handle errors, rather than using unwrap, in examples #52

Merged
merged 3 commits into from Dec 14, 2022

Conversation

LeoniePhiline
Copy link

@LeoniePhiline LeoniePhiline commented Dec 11, 2022

This PR

Fixes #49

Notes

A note regarding the from_path* example changes

I simplified these examples, as they did not plainly show off usage and were somewhat nonsensical:

  • They used an external crate, which was unnecessary.
  • They tried getting the home directory, then insinuated joining it with an absolute path.
  • They gave the false impression absolute paths were necessary, while relative paths are just as suitable.
  • They used .as_path() for a method accepting AsRef<Path>, which PathBuf implements.
  • The docs don't actually make clear what's the difference between from_filename* and from_path* (which is in fact the recursive lookup in parent directories). I'll open a separate issue about that to keep the PR clean.

A note regarding dotenv_override example changes

  • Replaced use by fully qualified name to keep consistent with the other dotenv* examples.
  • Removed .ok() for consistency with the dotenvy::dotenv example.

@LeoniePhiline LeoniePhiline changed the title fix(docs): Use question mark operator, rather than unwrap, in examples fix(docs): Use question mark operator ?, rather than unwrap, in examples Dec 11, 2022

fn main() {
// load environment variables from .env file
dotenv().expect(".env file not found");
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line notably produced misleading error messages, claiming the file was not found even in cases where it actually contained invalid variable declarations.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point

@LeoniePhiline LeoniePhiline changed the title fix(docs): Use question mark operator ?, rather than unwrap, in examples fix(docs): Use error handling, rather than unwrap, in examples Dec 11, 2022
@LeoniePhiline LeoniePhiline changed the title fix(docs): Use error handling, rather than unwrap, in examples fix(docs): Handle errors, rather than using unwrap, in examples Dec 11, 2022
@github-actions
Copy link

Code Coverage

@allan2
Copy link
Owner

allan2 commented Dec 12, 2022

For most of these, I prefer the original. See this comparison:

Original

dotenvy::from_filename("custom.env").unwrap();

Suggested

fn main() -> Result<(), Box<dyn std::error::Error>> {
dotenvy::from_filename("custom.env")?;
    Ok(())
}

The original is more legible for illustrating the usage of from_filename().

I can see ? being preferable to unwrap() in lengthier examples that are often copied and pasted, but that isn't usually the case here.

@LeoniePhiline
Copy link
Author

LeoniePhiline commented Dec 13, 2022

@allan2 You're missing here that rustdoc renders this as:

dotenvy::from_filename("custom.env")?;

The entire code block is being run as a test. Lines prefixed with # are removed from user-facing documentation.

This is common practice, as well as best practice in Rust projects.

I would recommend you run cargo doc --open and have a look at the generated documentation.


I can see ? being preferable to unwrap() in lengthier examples that are often copied and pasted, but that isn't usually the case here.

This isn't at all related to lengthiness, but about proliferating proper error handling.

I am very sure the examples shown in the dotenvy documentation are copy-pasted very, very often.

@allan2
Copy link
Owner

allan2 commented Dec 13, 2022

My apologies, I did miss that.

This PR looks good to me!

@sonro sonro merged commit a980410 into allan2:master Dec 14, 2022
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 this pull request may close these issues.

Use ? rather than .unwrap() in examples
3 participants