Make fields of ConnectionSetupRequestError public #1131
Conversation
rukai
changed the title
rukai
force-pushed
the
make_connection_setup_request_error_public
branch
from
a72651a to
bde3e60
Compare
|
|
| @@ -696,8 +696,8 @@ pub enum TranslationError { | |||
| #[derive(Error, Debug, Clone)] | |||
| #[error("Failed to perform a connection setup request. Request: {request_kind}, reason: {error}")] | |||
| pub struct ConnectionSetupRequestError { | |||
There was a problem hiding this comment.
Should we maybe also mark the struct as #[non_exhaustive]? @Lorak-mmk @wprzytula WDYT?
There was a problem hiding this comment.
Or introduce pub getters instead of pubifying the fields.
There was a problem hiding this comment.
Pub getters would prevent matching on errors
There was a problem hiding this comment.
Pub getters would prevent matching on errors
Well, you still could pattern match down until ConnectionSetupRequestError, and then you could indeed not pattern match deeper. Would it be a serious limitation?
There was a problem hiding this comment.
Well, consider this patch to upgrade our project to the latest release of scylla-rust-driver:
https://github.com/shotover/shotover-proxy/pull/1840/files#diff-3c1b291110379020fb6bc7d1644e2e0027997d5243b1c111d1a927a577f9a2c9
The patch uses this PR to scylla-rust-driver that makes the field public allowing this to work:
match err {
NewSessionError::ConnectionPoolError(ConnectionPoolError::Broken {
last_connection_error:
ConnectionError::ConnectionSetupRequestError(ConnectionSetupRequestError {
error: ConnectionSetupRequestErrorKind::DbError(DbError::ServerError, err),
..
}),
}) => assert_eq!(format!("{err}"), "expected error"),
_ => panic!("Unexpected error, was {err:?}"),
}Using the latest release of scylla-rust-driver we instead need to break it into two separate matches:
match err {
NewSessionError::ConnectionPoolError(ConnectionPoolError::Broken {
last_connection_error: ConnectionError::ConnectionSetupRequestError(err),
}) => match err.get_error() {
ConnectionSetupRequestErrorKind::DbError(DbError::ServerError, err) => {
assert_eq!(format!("{err}"), "expected error")
}
_ => panic!("Unexpected error, was {err:?}"),
},
_ => panic!("Unexpected error, was {err:?}"),
}So the current state of things is usable and enables breaking changes of the internal fields of the struct in the future.
On the other hand I strongly value being able to write a single match rather than having to break it up into two separate matches.
Its ultimately your call.
There was a problem hiding this comment.
Imo making the fields public and marking the struct #[non_exhaustive] should be the solution here.
This is consistent with our enums error types, which have tuple variants, so their fields are visible, and so we can add new variants, but not change existing ones.
Here similarly we will be able to add new fields (e.g. we may want to store some context in the future), but won't be able to edit existing fields.
As an exception, I'd keep structs like BrokenConnectionError with private fields. This is because the internal type can't be matched anyway since if first needs to be downcasted, for which we have the API.
There was a problem hiding this comment.
So, unless @wprzytula or @muzarski have strong objections, I'd ask you @rukai to add #[non_exhaustive] and then we'll merge it.
There was a problem hiding this comment.
So, unless @wprzytula or @muzarski have strong objections, I'd ask you @rukai to add
#[non_exhaustive]and then we'll merge it.
I'm fine with this approach
There was a problem hiding this comment.
that works for me!
I've pushed the changes.
|
Anything I can do to move this forward? |
| @@ -696,8 +696,8 @@ pub enum TranslationError { | |||
| #[derive(Error, Debug, Clone)] | |||
| #[error("Failed to perform a connection setup request. Request: {request_kind}, reason: {error}")] | |||
| pub struct ConnectionSetupRequestError { | |||
There was a problem hiding this comment.
Pub getters would prevent matching on errors
Well, you still could pattern match down until ConnectionSetupRequestError, and then you could indeed not pattern match deeper. Would it be a serious limitation?
This allows user to match on the error and handle the error accordingly.
rukai
force-pushed
the
make_connection_setup_request_error_public
branch
from
bde3e60 to
9bfd3c2
Compare
|
@wprzytula This can land in 0.15.1 |
…uest_error_public Make fields of ConnectionSetupRequestError public (cherry picked from commit affcf82)
Pre-review checklist
./docs/source/.Fixes:annotations to PR description.This seems like an accidental omission. Without these fields being public it is impossible to match on the error type.