peasee commented on code in PR #517:
URL: 
https://github.com/apache/arrow-rs-object-store/pull/517#discussion_r2463135228


##########
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:
   I'm not sure I agree. Most of the builders I have seen either don't provide 
a way to supply an option and `with_x` has the assumption you're setting a 
value, or they provide a secondary `set_x` to set the specific `Option<T>`. For 
example:
   
   * 
[`opentelemetry_sdk::metrics::MeterProviderBuilder`](https://docs.rs/opentelemetry_sdk/0.31.0/src/opentelemetry_sdk/metrics/meter_provider.rs.html#250)
 provides a `with_resource(mut self, resource: Resource)` where the underlying 
property is a `Option<Resource>`. It provides no alternative to set the option 
back to `None`
   * 
[`std::sys::unix::process::Command`](https://stdrs.dev/nightly/x86_64-unknown-linux-gnu/src/std/sys/unix/process/process_common.rs.html#248)
 stores a `cwd: Option<CString>` but only provides `cwd(&mut self, dir: 
&OsStr)` with no alternative to set the option back to `None`
   
   I'm not sure setting back to `None` is a common scenario, you should just 
build a new option?



-- 
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]

Reply via email to