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

README: display clear errors in the example #130

Merged
merged 4 commits into from
Nov 14, 2023
Merged

Conversation

nim65s
Copy link
Contributor

@nim65s nim65s commented Nov 12, 2023

Description

Hi,

Building the example in the README raise the following warning:

warning: unused import: `Query`
 --> src/main.rs:1:24
  |
1 | use influxdb::{Client, Query, Timestamp, ReadQuery};
  |                        ^^^^^
  |
  = note: `#[warn(unused_imports)]` on by default

And running it for me shows:

thread 'main' panicked at src/main.rs:35:5:
Write result was not okay

which is not super helpful.

With the proposed changes, I get:

Error writing result: authorization error. User not authorized

which is more clear :)

Checklist

  • Formatted code using cargo fmt --all
  • Linted code using clippy
    • with reqwest feature: cargo clippy --manifest-path influxdb/Cargo.toml --all-targets --no-default-features --features use-serde,derive,reqwest-client -- -D warnings
    • with surf feature: cargo clippy --manifest-path influxdb/Cargo.toml --all-targets --no-default-features --features use-serde,derive,hyper-client -- -D warnings
  • Updated README.md using cargo doc2readme -p influxdb --expand-macros
  • Reviewed the diff. Did you leave any print statements or unnecessary comments?
  • Any unfinished work that warrants a separate issue captured in an issue with a TODO code comment

Copy link
Collaborator

@msrd0 msrd0 left a comment

Choose a reason for hiding this comment

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

Please don't modify the readme file directly. The readme is generated from the documentation in the src/lib.rs file.

@nim65s
Copy link
Contributor Author

nim65s commented Nov 14, 2023

done, thanks for the review

Comment on lines 59 to 62
//! if let Err(e) = client.query(weather_readings).await {
//! println!("Error writing result: {e}");
//! return;
//! }
Copy link
Collaborator

@msrd0 msrd0 Nov 14, 2023

Choose a reason for hiding this comment

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

Why not use .expect()? I mean we don't actually run this particular doc test, but if we did, it would fail using assert/expect/panic/... but not when using println and return

//! println!("{}", read_result.unwrap());
//! match client.query(read_query).await {
//! Ok(read_result) => println!("{}", read_result),
//! Err(e) => println!("Error reading result: {e}"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not use .expect()?

@nim65s
Copy link
Contributor Author

nim65s commented Nov 14, 2023

@msrd0 : The point of this PR is to display a more precise error message to the end user, so we need to display {e} or something similar.

ie. to go from something like "Write result was not okay" to something like "Write result was not okay, because your user is not authorized". I don't think we can do this with .expect().

@nim65s
Copy link
Contributor Author

nim65s commented Nov 14, 2023

Alternatively, we could have main() returning a Result<(), influxdb::Error>, and use ? in read and write.

@msrd0
Copy link
Collaborator

msrd0 commented Nov 14, 2023

Is the difference between display and debug this big? I just looked it up, and expect("msg") is the same as unwrap_or_else(|e| panic!("msg: {e:?}")). I think given that this code is an example and has no intention to have "perfect" error handling, I'd go with either expect or unwrap and panic.

While your code outputs a nice error message to the terminal and exits, I think it is a bit more verbose than necessary for an example, and it needs a bit longer for a human to detect it as error-handling code. But I believe examples should be as concise as possible.

@msrd0
Copy link
Collaborator

msrd0 commented Nov 14, 2023

Using ? is also a good idea

@nim65s
Copy link
Contributor Author

nim65s commented Nov 14, 2023

Now I get Error: AuthorizationError, which is good enough for me to understand that I need to setup auth :)

@nim65s
Copy link
Contributor Author

nim65s commented Nov 14, 2023

Ok, I was wrong about the use of msg in expect(), thanks for the clarification. I'm open to use that too, as you prefer.

Copy link
Collaborator

@msrd0 msrd0 left a comment

Choose a reason for hiding this comment

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

Thanks!

@msrd0 msrd0 merged commit a7a56df into influxdb-rs:main Nov 14, 2023
19 of 23 checks passed
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.

None yet

2 participants