feat: Add a Builder struct and use it in from_[head|tail]_response #15
Conversation
The functions from_tail_response and from_head_response were marked async, but did no calls to .await.
torarvid
changed the title
|
Sweet! Thanks for taking this on it looks like a good start. |
src/lib.rs
Outdated
| let Some(client) = self.client else { | ||
| return Err(AsyncHttpRangeReaderBuilderError::MissingClient); | ||
| }; | ||
| let Some(url) = self.url else { | ||
| return Err(AsyncHttpRangeReaderBuilderError::MissingUrl); | ||
| }; |
There was a problem hiding this comment.
I would prefer that if these fields are not optional, that a user provides them when constructing the builder. That turns this error from a runtime error into a compile time error.
There was a problem hiding this comment.
Hi, and thanks for the comments 👍🏼 Yeah, I see the client being required is probably good to have a compile-time check for.
Regarding url the reason I made it optional was that I was thinking if one calls from_head_response, it will set the url (so a caller who might want the existing behavior where you extract the url from the headers in the HEAD response could have that). On the other hand, somebody who might want to use a HEAD response (to validate that the headers confirm that Range requests are supported) but at the same time not use the header-url, can use the builder url(...) method to do so. WDYT?
There was a problem hiding this comment.
I started by making client required. Awaiting to hear your reaction to my comment about url before doing anything there 😊
src/lib.rs
Outdated
| pub fn requested_range(mut self, requested_range: SparseRange) -> Self { | ||
| self.requested_range = requested_range; | ||
| self | ||
| } |
There was a problem hiding this comment.
This is only used internally when calling from_tail_response. It should not be set by the user.
There was a problem hiding this comment.
I ended up thinking the same about initial_tail_response (seems like only from_tail_response needs to call that?)
src/lib.rs
Outdated
| AsyncHttpRangeReader::builder() | ||
| .client(Client::new().into()) | ||
| .url(server.url().join("andes-1.8.3-pyhd8ed1ab_0.conda").unwrap()) | ||
| .build() | ||
| .expect("could not build reader"); |
There was a problem hiding this comment.
Im not sure if this actually works? This doesnt call any of the from_ methods which means the content_length is never set. Maybe you should also do a prefetch of a certain range followed by actually reading from the stream and comparing the data with the file on disk.
There was a problem hiding this comment.
Yeah, this test is possibly "too simplistic to be valuable". I'll think about that some more
There was a problem hiding this comment.
Just as a quick update, I made sure the build() method now checks that content_length != 0, and adjusted this test to make a head request. I will try to add additional tests as well.
This commit: * Removes derive(Default) from the builder * Adds builder::new that requires a client * Changes a few builder methods from pub to private * Makes builder from_head_response and from_tail_response methods instead of static functions * Adds a check that content_length is non-zero in the build() method
|
@baszalmstra I believe this PR is ready for a re-review based on your previous comments 👍🏼 |
| let Some(url) = self.url else { | ||
| return Err(AsyncHttpRangeReaderBuilderError::MissingUrl); | ||
| }; |
There was a problem hiding this comment.
Could you add this to new too? If a field is required anyway lets make sure a user is forced to add it at compile time.
| if self.content_length == 0 { | ||
| return Err(AsyncHttpRangeReaderBuilderError::InvalidContentLength); | ||
| } |
There was a problem hiding this comment.
If I understand it correctly this means you have to call either .from_head_response or .from_tail_response. If either of those are required lets make those the methods that create the builder. E.g.
AsyncHttpRangeReaderBuilder::from_tail_response(url, response)There was a problem hiding this comment.
Otherwise AsyncHttpRangeReaderBuilder::new().build() would result in a runtime error. Would be nicer if a that would be caught at compile time.
There was a problem hiding this comment.
Nice. I like this. Will change it
| let head_response = AsyncHttpRangeReader::initial_head_request( | ||
| Client::new(), | ||
| url.clone(), | ||
| HeaderMap::default(), | ||
| ) | ||
| .await | ||
| .expect("could not perform head request"); | ||
|
|
||
| let builder = AsyncHttpRangeReader::builder(Client::new().into()) | ||
| .from_head_response(head_response) | ||
| .expect("could not build reader from head response") | ||
| .build() | ||
| .expect("could not build reader"); |
There was a problem hiding this comment.
I think this API is slightly weird. You first create an object followed by creating a builder. I would expect to be able to do both operations through the builder.
Hello there 👋🏻 I was intrigued by the discussion in #11, and so I've made this PR to allow for a more Builder-like pattern of creating a reader instance. Part of my reason for the PR is just to get a bit of Rust practice. If this code is far away from what you would want in this repo, then I will not be offended 😄