alamb commented on code in PR #517:
URL:
https://github.com/apache/arrow-rs-object-store/pull/517#discussion_r2462735965
##########
examples/get.rs:
##########
@@ -0,0 +1,34 @@
+#[cfg(not(all(target_arch = "wasm32", target_os = "unknown")))]
+use object_store::{local::LocalFileSystem, path::Path, ObjectStore};
+#[cfg(not(all(target_arch = "wasm32", target_os = "unknown")))]
+use tempfile::tempdir;
+
+fn main() {
Review Comment:
Thank you @peasee -- this is great.
I am not sure how discoverable these examples will be (people have to know
to go into the code and check them out) How about putting them in the doc
comments
For example, we could put them directly on
https://docs.rs/object_store/latest/object_store/trait.ObjectStore.html#method.get
##########
src/lib.rs:
##########
@@ -1035,6 +1054,84 @@ impl GetOptions {
}
Ok(())
}
+
+ /// Create a new [`GetOptions`]
+ #[must_use]
+ pub fn new() -> Self {
+ Self::default()
+ }
+
+ /// Sets the `if_match` condition.
+ ///
+ /// See [`GetOptions::if_match`]
+ #[must_use]
+ pub fn with_if_match(mut self, etag: impl Into<String>) -> Self {
Review Comment:
With this API there is no way to unset the `if_match` field (set it back to
`None`)
I think it is more typical in builders to pass in the same type, something
like
```suggestion
pub fn with_if_match(mut self, etag: Option<impl Into<String>>) -> Self {
```
That would also reduce the number of API functions required by a factor of
2x
##########
src/lib.rs:
##########
@@ -324,6 +324,32 @@
//! # }
//! ```
//!
+//! To retrieve ranges from a versioned object, use [`ObjectStore::get_opts`]
by specifying the range in the [`GetOptions`].
+//!
+//! ```ignore-wasm32
Review Comment:
I am not familiar with this `ignore-wasm32` annotation -- it seems like it
may just not run these examples at all
If we are having trouble running the doc tests on wasm32, I suggest we just
don't run the doc examples on wasm32 in CI
##########
src/lib.rs:
##########
@@ -1035,6 +1054,84 @@ impl GetOptions {
}
Ok(())
}
+
+ /// Create a new [`GetOptions`]
+ #[must_use]
+ pub fn new() -> Self {
+ Self::default()
+ }
+
+ /// Sets the `if_match` condition.
+ ///
+ /// See [`GetOptions::if_match`]
+ #[must_use]
Review Comment:
Why the `must_use` annotation?
##########
src/buffered.rs:
##########
@@ -590,13 +590,7 @@ mod tests {
writer.write_all(&[0; 5]).await.unwrap();
writer.shutdown().await.unwrap();
let response = store
- .get_opts(
- &path,
- GetOptions {
- head: true,
- ..Default::default()
- },
- )
+ .get_opts(&path, GetOptions::new().with_head(true))
Review Comment:
❤️
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
