Replace panics with results & better option types #437
Conversation
|
Awesome work! @johan-bjareholt What do you think about the aw-client change? |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #437 +/- ##
==========================================
+ Coverage 70.58% 70.88% +0.29%
==========================================
Files 47 47
Lines 2917 2902 -15
==========================================
- Hits 2059 2057 -2
+ Misses 858 845 -13 ☔ View full report in Codecov by Sentry. |
| @@ -17,7 +17,7 @@ pub use aw_models::{Bucket, BucketMetadata, Event}; | |||
|
|
|||
| pub struct AwClient { | |||
| client: reqwest::Client, | |||
| pub baseurl: String, | |||
| pub baseurl: reqwest::Url, | |||
There was a problem hiding this comment.
Do we really want to expose our reqwest dependency in the API? I think we should avoid doing that.
Wouldn't it be better to have a custom error type that the Url is invalid?
There was a problem hiding this comment.
Is this really an API? While I found a old crates.io package, this project really does not feel like a library.
That said, if you'd like to keep that distinction, then I agree, but then I think new() should return a Result, which is fine, but a bit odd.
There was a problem hiding this comment.
It is an API, just not a very mature one :)
As you saw, it's used by aw-sync, but its also used by aw-watcher-window-wayland.
| let client = reqwest::Client::builder() | ||
| .timeout(std::time::Duration::from_secs(120)) | ||
| .build() | ||
| .unwrap(); | ||
| let hostname = gethostname::gethostname().into_string().unwrap(); |
There was a problem hiding this comment.
This unwrap is extremely unlikely, you would need to essentially have invalid utf-8 in your hostname to fail here.
Keep this functionality, but maybe replace the "unwrap" with an "expect" instead.
There was a problem hiding this comment.
I agree, very unlikely, but in that unlikely event, panic-ing doesn't make sense, I think. Maybe I should just strip out UTF-8 characters instead?
|
Thanks for the great PR @nathanmerrill! ❤️ |
I tried to remove as many unwraps(), panics()!, etc, with Result<>. I did not go deep into the core sync logic, as I'm not as comfortable there.
I move a bunch of the parsing out of the main function and into arg-specific functions. That way, clap can provide all the context needed for an invalid argument. I also replaced "String" with more specific types in the clap commands.
AwClient was the most "substantial" change here. It's new function had several points where it would panic out. Therefore, I replaced its constructor arguments with types that prevented these panics.
Instead of taking a host+port, it just takes a reqwest::Url. This prevents the panic if the url is malformed. Additionally, it would try to read the hostname() which would also panic if the hostname used non-UTF-8 characters, so the hostname is a new parameter to the function. (I did consider rewriting get_hostname() to a non-panicing version that just strips all non-UTF-8 characters from the hostname. If you prefer this, I'm happy to update)