Re: [PR] docs(client): improve warnings and links in client doc comments [arrow-rs-object-store]

2025-11-21 Thread via GitHub 


kylebarron merged PR #540:
URL: https://github.com/apache/arrow-rs-object-store/pull/540


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

Re: [PR] docs(client): improve warnings and links in client doc comments [arrow-rs-object-store]

2025-11-21 Thread via GitHub 


alamb commented on PR #540:
URL: 
https://github.com/apache/arrow-rs-object-store/pull/540#issuecomment-3563443570

   I took the liberty of running `cargo fmt` and pushing to this branch to get 
the CI to pass cleanly


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

Re: [PR] docs(client): improve warnings and links in client doc comments [arrow-rs-object-store]

2025-11-21 Thread via GitHub 


alamb commented on PR #540:
URL: 
https://github.com/apache/arrow-rs-object-store/pull/540#issuecomment-3563432434

   > Sorry for the long wait from my side
   
   No problems! Thank you for the contribution


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

Re: [PR] docs(client): improve warnings and links in client doc comments [arrow-rs-object-store]

2025-11-20 Thread via GitHub 


CommanderStorm commented on code in PR #540:
URL: 
https://github.com/apache/arrow-rs-object-store/pull/540#discussion_r2548352265


##
src/client/mod.rs:
##
@@ -488,46 +492,76 @@ impl ClientOptions {
 self
 }
 
-/// Sets what protocol is allowed. If `allow_http` is :
-/// * false (default):  Only HTTPS are allowed
-/// * true:  HTTP and HTTPS are allowed
+/// Sets what protocol is allowed.
+///
+/// If `allow_http` is :
+/// * `false` (default):  Only HTTPS are allowed
+/// * `true`:  HTTP and HTTPS are allowed
 pub fn with_allow_http(mut self, allow_http: bool) -> Self {
 self.allow_http = allow_http.into();
 self
 }
 /// Allows connections to invalid SSL certificates
-/// * false (default):  Only valid HTTPS certificates are allowed
-/// * true:  All HTTPS certificates are allowed
 ///
-/// # Warning
+/// If `allow_invalid_certificates` is :
+/// * `false` (default):  Only valid HTTPS certificates are allowed
+/// * `true`:  All HTTPS certificates are allowed
+///
+/// 
+///
+/// **Warning**
 ///
 /// You should think very carefully before using this method. If
 /// invalid certificates are trusted, *any* certificate for *any* site
 /// will be trusted for use. This includes expired certificates. This
 /// introduces significant vulnerabilities, and should only be used
 /// as a last resort or for testing
+///
+/// 
 pub fn with_allow_invalid_certificates(mut self, allow_insecure: bool) -> 
Self {
 self.allow_insecure = allow_insecure.into();
 self
 }
 
-/// Only use http1 connections
+/// Only use HTTP/1 connections
+///
+/// # See Also
+/// * [`Self::with_http2_only`] if you only want to use HTTP/2
+/// * [`Self::with_allow_http2`] if you want to use HTTP/1 or HTTP/2
 ///
-/// This is on by default, since http2 is known to be significantly slower 
than http1.
+/// 
+/// This is off by default, since HTTP/2 is known to be significantly 
slower than http1.
+/// 
 pub fn with_http1_only(mut self) -> Self {
 self.http2_only = false.into();
 self.http1_only = true.into();
 self
 }
 
-/// Only use http2 connections
+/// Only use HTTP/2 connections
+///
+/// # See Also
+/// * [`Self::with_http1_only`] if you only want to use HTTP/1
+/// * [`Self::with_allow_http2`] if you want to use HTTP/1 or HTTP/2
+///
+/// 
+/// This is off by default, since HTTP/2 is known to be significantly 
slower than HTTP/1.

Review Comment:
   ```suggestion
   /// HTTP/2 is not used by default. See details 
[#104](https://github.com/apache/arrow-rs-object-store/issues/104)
   ```



##
CHANGELOG-old.md:
##
@@ -475,7 +475,7 @@
 
 - Remove deprecated try\_with\_option methods 
[\#5237](https://github.com/apache/arrow-rs/pull/5237) 
[[object-store](https://github.com/apache/arrow-rs/labels/object-store)] 
([tustvold](https://github.com/tustvold))
 - object\_store: full HTTP range support 
[\#5222](https://github.com/apache/arrow-rs/pull/5222) 
[[object-store](https://github.com/apache/arrow-rs/labels/object-store)] 
([clbarnes](https://github.com/clbarnes))
-- feat\(object\_store\): use http1 by default 
[\#5204](https://github.com/apache/arrow-rs/pull/5204) 
[[object-store](https://github.com/apache/arrow-rs/labels/object-store)] 
([wjones127](https://github.com/wjones127))
+- feat\(object\_store\): use HTTP/1 by default 
[\#5204](https://github.com/apache/arrow-rs/pull/5204) 
[[object-store](https://github.com/apache/arrow-rs/labels/object-store)] 
([wjones127](https://github.com/wjones127))

Review Comment:
   ```suggestion
   - feat\(object\_store\): use http1 by default 
[\#5204](https://github.com/apache/arrow-rs/pull/5204) 
[[object-store](https://github.com/apache/arrow-rs/labels/object-store)] 
([wjones127](https://github.com/wjones127))
   ```



##
src/client/mod.rs:
##
@@ -639,15 +673,15 @@ impl ClientOptions {
 /// Sets a timeout for receiving an acknowledgement of the keep-alive ping.
 ///
 /// If the ping is not acknowledged within the timeout, the connection 
will be closed.
-/// Does nothing if http2_keep_alive_interval is disabled.
+/// Does nothing if HTTP/2_keep_alive_interval is disabled.

Review Comment:
   ```suggestion
   /// Does nothing if `http2_keep_alive_interval` is disabled.
   ```



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

Re: [PR] docs(client): improve warnings and links in client doc comments [arrow-rs-object-store]

2025-11-20 Thread via GitHub 


alamb commented on code in PR #540:
URL: 
https://github.com/apache/arrow-rs-object-store/pull/540#discussion_r2546613596


##
src/client/mod.rs:
##
@@ -488,46 +492,76 @@ impl ClientOptions {
 self
 }
 
-/// Sets what protocol is allowed. If `allow_http` is :
-/// * false (default):  Only HTTPS are allowed
-/// * true:  HTTP and HTTPS are allowed
+/// Sets what protocol is allowed.
+///
+/// If `allow_http` is :
+/// * `false` (default):  Only HTTPS are allowed

Review Comment:
   ```suggestion
   /// * `false` (default):  Only HTTPS is allowed
   ```



##
src/client/mod.rs:
##
@@ -488,46 +492,76 @@ impl ClientOptions {
 self
 }
 
-/// Sets what protocol is allowed. If `allow_http` is :
-/// * false (default):  Only HTTPS are allowed
-/// * true:  HTTP and HTTPS are allowed
+/// Sets what protocol is allowed.
+///
+/// If `allow_http` is :
+/// * `false` (default):  Only HTTPS are allowed
+/// * `true`:  HTTP and HTTPS are allowed
 pub fn with_allow_http(mut self, allow_http: bool) -> Self {
 self.allow_http = allow_http.into();
 self
 }
 /// Allows connections to invalid SSL certificates
-/// * false (default):  Only valid HTTPS certificates are allowed
-/// * true:  All HTTPS certificates are allowed
 ///
-/// # Warning
+/// If `allow_invalid_certificates` is :
+/// * `false` (default):  Only valid HTTPS certificates are allowed
+/// * `true`:  All HTTPS certificates are allowed
+///
+/// 
+///
+/// **Warning**
 ///
 /// You should think very carefully before using this method. If
 /// invalid certificates are trusted, *any* certificate for *any* site
 /// will be trusted for use. This includes expired certificates. This
 /// introduces significant vulnerabilities, and should only be used
 /// as a last resort or for testing
+///
+/// 
 pub fn with_allow_invalid_certificates(mut self, allow_insecure: bool) -> 
Self {
 self.allow_insecure = allow_insecure.into();
 self
 }
 
-/// Only use http1 connections
+/// Only use HTTP/1 connections

Review Comment:
   ```suggestion
   /// Only use HTTP/1 connections (default)
   ```



##
src/client/mod.rs:
##
@@ -639,15 +673,15 @@ impl ClientOptions {
 /// Sets a timeout for receiving an acknowledgement of the keep-alive ping.
 ///
 /// If the ping is not acknowledged within the timeout, the connection 
will be closed.
-/// Does nothing if http2_keep_alive_interval is disabled.
+/// Does nothing if HTTP/2_keep_alive_interval is disabled.

Review Comment:
   agreed



##
src/client/mod.rs:
##
@@ -488,46 +492,76 @@ impl ClientOptions {
 self
 }
 
-/// Sets what protocol is allowed. If `allow_http` is :
-/// * false (default):  Only HTTPS are allowed
-/// * true:  HTTP and HTTPS are allowed
+/// Sets what protocol is allowed.
+///
+/// If `allow_http` is :
+/// * `false` (default):  Only HTTPS are allowed
+/// * `true`:  HTTP and HTTPS are allowed
 pub fn with_allow_http(mut self, allow_http: bool) -> Self {
 self.allow_http = allow_http.into();
 self
 }
 /// Allows connections to invalid SSL certificates
-/// * false (default):  Only valid HTTPS certificates are allowed
-/// * true:  All HTTPS certificates are allowed
 ///
-/// # Warning
+/// If `allow_invalid_certificates` is :
+/// * `false` (default):  Only valid HTTPS certificates are allowed
+/// * `true`:  All HTTPS certificates are allowed
+///
+/// 
+///
+/// **Warning**
 ///
 /// You should think very carefully before using this method. If
 /// invalid certificates are trusted, *any* certificate for *any* site
 /// will be trusted for use. This includes expired certificates. This
 /// introduces significant vulnerabilities, and should only be used
 /// as a last resort or for testing
+///
+/// 
 pub fn with_allow_invalid_certificates(mut self, allow_insecure: bool) -> 
Self {
 self.allow_insecure = allow_insecure.into();
 self
 }
 
-/// Only use http1 connections
+/// Only use HTTP/1 connections
+///
+/// # See Also
+/// * [`Self::with_http2_only`] if you only want to use HTTP/2
+/// * [`Self::with_allow_http2`] if you want to use HTTP/1 or HTTP/2
 ///
-/// This is on by default, since http2 is known to be significantly slower 
than http1.
+/// 
+/// This is off by default, since HTTP/2 is known to be significantly 
slower than http1.
+/// 
 pub fn with_http1_only(mut self) -> Self {
 self.http2_only = false.into();
 self.http1_only = true.into();
 self
 }
 
-/// Only use http2 connections
+/// Only use HTTP/2 connections
+///
+/// # See Al

Re: [PR] docs(client): improve warnings and links in client doc comments [arrow-rs-object-store]

2025-11-20 Thread via GitHub 


alamb commented on PR #540:
URL: 
https://github.com/apache/arrow-rs-object-store/pull/540#issuecomment-3558762625

   Love it -- thank you @CommanderStorm and @kylebarron 


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