-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Allow for multiple implementations of PgMetadataLookup #2639
Allow for multiple implementations of PgMetadataLookup #2639
Conversation
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.
Thanks for this PR 👍
I've left a few small comments what should be improved/discussed.
Otherwise this should have a entry in our changelog. (Just add a line to the corresponding section of Changelog.md
.
As a side note, I just realized that supporting user defined types with pure async would require a async variant of the |
And AST |
That's a different topic. Let's discuss one thing at the time. I will try to open a topic at the discussion part of this repo, where we could discuss how to generally deal with designing an async API. (Will update this comment as soon as there is something to link) |
@@ -159,37 +159,37 @@ where | |||
} | |||
|
|||
impl HasSqlType<Int4range> for Pg { | |||
fn metadata(_: &PgMetadataLookup) -> PgTypeMetadata { | |||
fn metadata(_: &Self::MetadataLookup) -> PgTypeMetadata { |
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.
Just as sanity check: These changes should not be required, right?
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.
Yes, we would have to change this because the new type is dyn PgMetadataLookup
. I got a compilation error when just putting dyn PgMetadataLookup
in HasSqlType<Array<T>>
because the recursive call required 'static
and it wasn't for some reason. Changing it to Self::MetadataLookup
fixed the error some how. I thinking this is easier to refactor anyway so I changed all the HasSqlType
implements for Pg
to Self::MetadataLookup
.
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.
That sounds for me like we should require that PgMetadataLookup
is always 'static
.
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.
I think it is implied to be 'static
as part of the type of the trait (kind of like Sized
if you don have ?Sized
) but not when in the function signature. I don't know why an error don't occur on the function signature though. I might post something on rust users forum to see if anyone understands what is going on.
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.
Looks fine from ,my side, at least for the general implementation. The documentation error needs to be fixed + I would like to have a changelog entry for this (Just add a entry into Changelog.md
.)
Otherwise I would wait a few days for @Ten0 to have a chance to voice potential problems here
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.
A final batch of small stylistic notes, otherwise this is good to go 👍
1943646
to
ec36620
Compare
This is inspired by part of #2257. It is simpler and also removes some unsafe code. This will also allow for multiple implementations of
Connection
s for Postgres.