Port from reqwest to ureq #205
Conversation
|
Thanks for this @TomFryersMidsummer! As I note in the issue, I don't consider the proj network use case as requiring async http, so reqwest was not a conscious choice and I'm happy to see it replaced. |
src/network.rs
Outdated
| res.into_reader() | ||
| .take(size_to_read as u64) | ||
| .read_to_end(&mut buf) | ||
| .map_err(|_| ProjError::ReadError)?; |
There was a problem hiding this comment.
I think we should propagate the error here, like for NetworkError.
| #[cfg(feature = "network")] | ||
| BuilderError(#[from] reqwest::Error), | ||
| NetworkError(Box<ureq::Error>), |
There was a problem hiding this comment.
Let's mark ProjError as #[non_exhaustive], so we don't take a breaking change next time we add a variant.
| // Copy the downloaded bytes into the buffer so it can be passed around | ||
| let capacity = contentlength.min(size_to_read); |
There was a problem hiding this comment.
This is a pre-existing issue, but we aren't consistent about out_size_read. If Content-Length is larger than size_to_read, we set out_size_read to a value larger than the actual length of the buffer. We should just set it to buf.len(), regardless or how we handle that case.
Same for _network_open of course.
There was a problem hiding this comment.
Thanks for the PR. I agree with @lnicola's feedback as well.
| @@ -19,7 +19,7 @@ geo-types = { version = "0.7.10", optional = true } | |||
| libc = "0.2.119" | |||
| num-traits = "0.2.14" | |||
| thiserror = "1.0.30" | |||
| reqwest = { version = "0.12.0", optional = true, default-features = false, features = ["blocking", "rustls-tls"] } | |||
| ureq = { version = "2.0.0", optional = true } | |||
There was a problem hiding this comment.
Did you actually test this on 2.0.0? Otherwise please specify whatever version you tested on (latest is 2.9.7).
Otherwise it's hard to know if you are leveraging any functionality that was added in a minor release.
There was a problem hiding this comment.
Yes, I tested it on 2.0.0.
|
Thank you for the feedback. I believe I've addressed everything now |
|
I believe |
|
Yes, sorry – missed that. I’ve fixed it now. |
|
Looks good, at least looking at the new commits, but I'll let someone else merge it since I've never used this crate. |
The network feature? Hard to imagine it going anywhere: the grid download functionality is a big step forward; without it you have to ship a couple of hundred MBs around with every libproj (which we wouldn't be able to do bc of crates.io limitations, even if we wanted to), and older versions have out-of-date grids leading to less-accurate conversions etc. |
|
Oops maybe I'm mixing up the network functionality in a different crate, disregard! |
Addresses georust#189, reducing `cargo tree -F network | wc -l` from 296 to 190.
Addresses #189, reducing
cargo tree -F network | wc -lfrom 296 to 190.