download a stream of bytes, not single byte #123
Conversation
| if resp.status() == StatusCode::NOT_FOUND { | ||
| Err(crate::Error::Other(resp.text().await?)) | ||
| } else { | ||
| Ok(resp.error_for_status()?.bytes().await?.to_vec()) | ||
| Ok(resp.error_for_status()?) | ||
| } |
There was a problem hiding this comment.
I created a non-pub helper function download_response to unify the common download code paths. I also noticed checking for StatusCode::NOT_FOUND was only done for download, not download_streamed. I think it makes more sense to have that unified here, but it might be a breaking change?
Up for you to decide, I'm happy to move that error handling around.
Usability note: Often the google error responses include helpful content that is being discarded here. I think on http status errors, we should actually try to convert it into an Error::Google(GoogleErrorResponse). Maybe in a separate MR?
| .bytes() | ||
| .await? | ||
| .into()) | ||
| } |
There was a problem hiding this comment.
The actual methods are now very straightfoward
|
Actually, I noticed that I not only need the stream of bytes but also at least the content-length for my use case (which is provided in a header). I will update my MR accordingly. |
|
Nevermind my last comment, please review this and merge if you feel this is appropriate. |
|
Hi! This PR is open since October and is +96 −32 across 3 files and I think it's pretty straightforward. I would appreciate you taking the time to review it, but of course I understand if you cannot make the time to do it. If you think this PR does not improve your codebase, please close it. Thank you! |
Hi!
I noticed that fact that your download_streaming APIs return an
impl Stream<Item = crate::Result<u8>>which is very inefficient.create::Result<u8>has size 72 on my machine (64bit arm), so for every byte copied, we need to pass 72 bytes around. But even if it wasimpl Stream<Item = u8>, it would still be inefficient. Such streams always work on slices of bytes (e.g. see the Read or AsyncRead traits) or here, it's the easiest and most idiomatic to just return a stream ofBytes.So I deprecated
download_streamedand addeddownload_bytes_stream.Hope you like it :)
Cheers,
Kosta