13

I am using the Diesel ORM wrapper with PostgreSQL. I was following the guide on their website which has the following code:

pub fn establish_connection() -> PgConnection {
     dotenv().ok();

     let database_url = env::var("DATABASE_URL")
         .expect("DATABASE_URL must be set");
     PgConnection::establish(&database_url)
         .expect(&format!("Error connecting to {}", database_url))
}

I understood what dotenv() does through the dotenv docs — it loads the env file. In the source code I saw that that dotenv() returns a Result. What does ok() do then? Does it unwrap the result? If so, why not use unwrap()?

Shepmaster
  • 388,571
  • 95
  • 1,107
  • 1,366
elementory
  • 133
  • 3
  • 7
  • see [the docs](https://doc.rust-lang.org/std/result/enum.Result.html) for the standard lib version. diesel's will probably be defined in terms of that. The [ok method](https://doc.rust-lang.org/std/result/enum.Result.html#method.ok) will probably do the same. `unwrap` can `panic`, but `ok` can't, so the caller can handle the failure case – joel Jun 24 '20 at 02:00
  • 1
    @joelb Note that the use of `ok()` is not for handling the failure case, it's for **ignoring** the failure case. When handling failure, one would not typically call `ok()` (because it throws away the error information), but match the returned `Result` directly. – user4815162342 Jun 24 '20 at 08:09

1 Answers1

9

It's a way to ignore errors arising from failing to load the dotenv environment file.

dotenv() returns a Result. Result::ok converts the Result into an Option. This Option does not trigger the warning about an unused Result.

why not use unwrap()

Because you don't want it to fail. In production, you should not have an environment file, instead you will use actual environment variables. If you unwrapped, then your service would fail in production immediately. This has happened to me, unfortunately.

Shepmaster
  • 388,571
  • 95
  • 1,107
  • 1,366
  • 1
    Would you consider it good practice to use `ok()` to silence the warning in production code? It seems like it will also silently ignore e.g. syntax error in the file (in case of the [`LineParse`](https://docs.rs/dotenv/0.15.0/dotenv/enum.Error.html#variant.LineParse) variant of the error result), or system errors other than "file not found" (e.g. not being able to read the file due to invalid permissions) and which you might want to know about. – user4815162342 Jun 24 '20 at 08:06
  • 1
    @user4815162342 in _general_ no, I don't think it's good to ignore errors this way. For `dotenv`, ignoring errors seems acceptable as the negative outcomes should only happen in development, where the stakes are lower. – Shepmaster Jun 24 '20 at 12:36
  • I see what you mean, the idea is that it's a non-problem in production. I wonder if a document as widely used (and copied/cited) as Diesel guide should be promoting the pattern. Perhaps it should at least display the error, as with `dotenv().map_err(|e| eprintln!("dotenv: {}", e)).ok()`. (I'm not sure if that will print an error when the file is not present.) If there is agreement as to a better idiom (if any), I can send them a PR to change the guide. – user4815162342 Jun 24 '20 at 12:48
  • @user4815162342 A problem I foresee is that Diesel doesn't want to concentrate on dotenv, but rather on the Diesel-specific code. Making the error handling for dotenv longer would distract from that purpose. A different path would be to modify dotenv so that "fatal" errors are reported as such and "warning" errors can be ignored. That has the downside of requiring the library to have an opinion on what is / isn't fatal. – Shepmaster Jun 24 '20 at 12:55