alamb commented on code in PR #580:
URL:
https://github.com/apache/arrow-rs-object-store/pull/580#discussion_r2632103474
##########
src/client/retry.rs:
##########
@@ -122,7 +122,10 @@ pub enum RequestError {
#[error("Server returned error response: {body}")]
Response { status: StatusCode, body: String },
- #[error(transparent)]
+ // We need to use `{0}` here rather than `error(transparent)` to ensure
that
+ // `Error::source` returns `HttpError` rather than the underlying
+ // `reqwest::Error` which would happen with `error(transparent)`.
+ #[error("{0}")]
Review Comment:
I found this change confusing given the docs on
https://docs.rs/thiserror/latest/thiserror/#derives
> Errors may use error(transparent) to forward the source and
[Display](https://doc.rust-lang.org/core/fmt/trait.Display.html) methods
straight through to an underlying error without adding an additional message.
This would be appropriate for enums that need an “anything else” variant.
Specifically the documentation mostly discusses the display implications.
However, I verified that the newly added test fails without this change:
```
thread 'client::retry::tests::test_retry' (1398936) panicked at
src/client/retry.rs:745:9:
HttpError not found in source chain
```
So in summary nice 🕵️ work @carlsverre (👋 btw -- it is great to see you on
github)
--
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]
