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.

Sign up for GitHub

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

Jump to bottom

Fix issue #1431 #1432

Merged
merged 1 commit into from
Sep 10, 2021
Merged

Fix issue #1431 #1432

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions sqlx-cli/src/bin/cargo-sqlx.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use clap::{crate_version, AppSettings, FromArgMatches, IntoApp};
use console::style;
use dotenv::dotenv;
use sqlx_cli::Opt;
use std::{env, process};

Expand All @@ -9,6 +10,7 @@ async fn main() {
// so we want to notch out that superfluous "sqlx"
let args = env::args_os().skip(2);

dotenv().ok();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
dotenv().ok();
let _ = dotenv();

Using .ok() to discard errors is not particularily idiomatic. This would have been fixed in dotenv's examples already if it was actively maintained.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jplatte thanks for your suggestion. Some follow up questions:

To what degree is this a style concern? A behavior concern?

Using .ok() to discard errors is not particularly idiomatic.

Why?

In particular, has the Clippy community discussed this? Has there been a consensus or something close to it? Here's what I've found that is somewhat related: Warn about blanket let _ =

I don't have a very strong preference here, but I lean towards preferring using ok() as above. For example, I think I agree with this part of tshepang's comment:

I ... think dotenv.ok() makes it clear enough that failure is acceptable

P.S. If you are interested in how I think about this more generally, see What does 'idiomatic' mean in Rust?

Copy link
Contributor

@jplatte jplatte Sep 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Behavior is the exact same, this is just a style concern. I've left a comment on the clippy issue you linked with my arguments for let _ = <expr>; over <expr>.ok();.

Copy link
Contributor

Choose a reason for hiding this comment

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

Found another issue: rust-lang/rust#66116

Copy link
Collaborator

Choose a reason for hiding this comment

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

We use .ok() a lot internally; IMO it's quicker to use because it's auto-suggested by my IDE as soon as I type .o which is personally important to me when setting up boilerplate like this.

In the discussions linked here, it seems like the jury is still out on the issue of what's idiomatic and what's not in terms of discarding results, so I don't think it's necessary to spend much time bikeshedding. Just use .ok() for now and if that method gains a #[must_use] later, or Clippy starts linting against it, then we can cargo fix that easily.

let matches = Opt::into_app()
.version(crate_version!())
.bin_name("cargo sqlx")
Expand Down
2 changes: 2 additions & 0 deletions sqlx-cli/src/bin/sqlx.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
use clap::{crate_version, FromArgMatches, IntoApp};
use console::style;
use dotenv::dotenv;
use sqlx_cli::Opt;

#[tokio::main]
async fn main() {
dotenv().ok();
let matches = Opt::into_app().version(crate_version!()).get_matches();

// no special handling here
Expand Down
3 changes: 0 additions & 3 deletions sqlx-cli/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
use anyhow::Result;
use dotenv::dotenv;

use crate::opt::{Command, DatabaseCommand, MigrateCommand};

Expand All @@ -13,8 +12,6 @@ mod prepare;
pub use crate::opt::Opt;

pub async fn run(opt: Opt) -> Result<()> {
dotenv().ok();

match opt.command {
Command::Migrate(migrate) => match migrate.command {
MigrateCommand::Add {
Expand Down