move from curl to reqwest #61
Conversation
|
Apparently this breaks rust 1.28 support. How important is that? |
|
Very nice! Yeah, I think when I created this program reqwest didn't exist yet 🙂
I would like to give some stability guarantees, but so far it has been quite arbitrary. We did want features that were not in Rust 1.0, but whether we pick Rust 1.10, 1.21 or 1.28 is arbitrary. Therefore I think it would be good to pick the first Rust 2018 version (being 1.31) and stick to that for a while. It would also allow us to opt-in to the Rust 2018 features. Feel free to bump the minimal required Rust version to 1.30 for now! Make sure to update the reference in the README too. |
|
Taking 1.31 once that is released sounds very good to me. |
src/cache.rs
Outdated
| } | ||
| let mut resp = reqwest::get(&self.url)?; | ||
| let mut buf: Vec<u8> = vec![]; | ||
| resp.copy_to(&mut buf)?; |
There was a problem hiding this comment.
The unused_results linter complains here. To avoid, use let _bytes = resp.copy_to(&mut buf)?;.
There was a problem hiding this comment.
What do you think about logging it instead? It's not quite the same, but it is getting rid of the linter warning and shouldn't hurt anyway.
There was a problem hiding this comment.
Ah, yeah, why not. Something like "Downloaded {} bytes".
There was a problem hiding this comment.
I worded it slightly differently, "{} bytes downloaded", but the result is the same ^^
|
I don't understand the CI failure here.. The log line shouldn't change anything about the behavior when updating the cache, and the previous commit passed those tests. |
|
@jcgruenhage since you run the command inside the Solution: Assign the number of bytes to a variable and log that. |
|
Oh, okay. Stupid me. I didn't try without (debug) logging |
|
Interesting, github now shows force pushes. Neat! |
Yeah, I noticed that too yesterday. Very nice. Looks good now, thanks! |
|
Merged in d950700 |
Fixes #60