influxdb-rs / influxdb-rust

Rust Client for the InfluxDB Time Series Database
https://crates.io/crates/influxdb
MIT License
256 stars 78 forks source link

README: display clear errors in the example #130

Closed nim65s closed 11 months ago

nim65s commented 11 months ago

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

nim65s commented 11 months ago

done, thanks for the review

nim65s commented 11 months ago

@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 commented 11 months ago

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

msrd0 commented 11 months ago

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 commented 11 months ago

Using ? is also a good idea

nim65s commented 11 months ago

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

nim65s commented 11 months ago

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.