-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: lift type mappings into driver crates #2970
Conversation
@@ -186,7 +192,7 @@ impl_database_ext! { | |||
Vec<sqlx::postgres::types::PgRange<sqlx::types::chrono::DateTime<_>>>, | |||
|
|||
#[cfg(feature = "chrono")] | |||
&[sqlx::postgres::types::PgRange<sqlx::types::chrono::DateTime<sqlx::types::chrono::Utc>>] | | |||
Vec<sqlx::postgres::types::PgRange<sqlx::types::chrono::DateTime<sqlx::types::chrono::Utc>>> | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is theoretically a breaking change but this only affects code that wouldn't have compiled anyways as &[PgRange<DateTime<Utc>>]
doesn't implement Decode
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, so because this change requires a type to actually implement Encode
or Decode
I can't really get around this.
966ad1c
to
f6ded3d
Compare
5d114e4
to
999ae76
Compare
While I doubt anyone's actually using the type mappings I had to change (to actually get this to compile), it's still technically a breaking change and will need to wait for the next major release. On the upside, it's possible to catch mistakes like this in the future because the macro invocation will complain about the lack of a |
/// If `value` is a well-known type, decode and format it using `Debug`. | ||
/// | ||
/// If `value` is not a well-known type or could not be decoded, the reason is printed instead. | ||
fn fmt_value_debug(value: &<Self as Database>::Value) -> FmtValue<'_, Self>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the reference to Value
be replaced with ValueRef
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason why it doesn't is explained at line 59.
It's feasible to add a Clone
impl to SqliteValueRef
but I would almost prefer to add like a reborrow()
method to ValueRef
instead, but either of those changes were rather out of scope for this PR.
Since it turned out to be a breaking change anyway, I'd be inclined to revisit that.
Motivated by #2917
Motivated by #2917
cc @g-bartoszek you can build your changes onto this. Just use
Postgres::fmt_value_debug()