Conversation
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
| let download = Download::new(&url, &file, self.process).with_hasher(&mut hasher); | ||
|
|
||
| let download = match status { | ||
| Some(status) => download.with_status(status), |
There was a problem hiding this comment.
Nit: A common builder trick is to let .with_foo() accept impl Into<Option<Foo>> so it can take foo, Some(foo) and None at the same time. Hopefully this way the chain can be more readable here 🙏
There was a problem hiding this comment.
Hmm, not sure I love it. I like the added precision of the builder method exactly taking the thing (not the thing wrapped in Option), and I don't mind this boilerplate so much -- while it's more verbose, it still seems still pretty straightforward to understand.
| && let Some(h) = self.hasher.as_ref() | ||
| { | ||
| h.borrow_mut().update(data); | ||
| } |
There was a problem hiding this comment.
Was the h.borrow_mut().update(data) call (previously inside the callback) moved elsewhere when the callback was removed, or is it no longer necessary?
The hasher is indeed initialized, via with_hasher(), but it does not appear to receive any bytes.
That said, shouldn't hash verification always fail?
| if resume_from != 0 { | ||
| req = req.header(header::RANGE, format!("bytes={resume_from}-")); | ||
| } | ||
| let res = req.send().await.context("error downloading file")?; |
There was a problem hiding this comment.
Shouldn't this be .map_err(DownloadError::Reqwest) instead?
Previously, request() returned Result<Response, DownloadError>, so any reqwest::Error was automatically wrapped as DownloadError::Reqwest before being propagated.
As it stands, this change breaks the is_network_failure() function, and likely the network_failure_does_not_delete_partial_file test as well.
Without the wrapping, the error can no longer be downcast to DownloadError, causing is_network_failure to always return false, and thus, always removing partial files.
4 participants
No description provided.