-
-
Notifications
You must be signed in to change notification settings - Fork 503
Derive Generic FromSql/ToSql #931
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
Conversation
struct InventoryItem<T: FromSqlOwned, U> | ||
where | ||
U: FromSqlOwned, |
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.
Are these bounds actually required?
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.
FromSqlOwned
is required for #[derive(FromSql)]
which is needed to run the test. Or were you asking about the derived Debug
and PartialEq
?
Edit: I also put the bounds in the two different positions to test that bounds and where clauses work as expected.
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.
Derive logic should normally insert those bounds on the generated impls as necessary - you don't need where T: Clone
at the struct definition when using #[derive(Clone)]
for example.
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 to make sure I understand the change request, the idea is add a FromSql<'a> bound on any generic type in the FromSql derive logic?
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.
Yep.
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.
So I started implementing this, and ran into an ambiguity (where this differs from Clone
, which is in the rust prelude). I need to create a path to the FromSql and ToSql traits, however it could be one of a few options:
- From the postgres_derive crate (
postgres_derive::FromSql<'a, T>
, but I think most uses will not import this crate directly) - From the postgres_types crate (
postgres_types::FromSql<'a, T>
) - Some other crate that re-exports or renames those types (no way to use a path from a crate name)
- Assume that it's
use
d somewhere (FromSql<'a, T>
)
Any suggestions on which assumption to use?
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.
Here's how the path is already produced in the existing logic: https://github.com/sfackler/rust-postgres/blob/master/postgres-derive/src/fromsql.rs#L90
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 pointing that out. Based on that, looks like it was option (2). Branch is updated, and tests pass locally.
227bb65
to
c656f12
Compare
Updated to fix the two issues (lint and failing test). |
3f68e9f
to
68d028f
Compare
Hope this gets in soon, derive is breaking on any struct thats borrowed/lifetimes |
Sorry for the delay - LGTM other than the clippy failures! |
68d028f
to
3827b2e
Compare
@sfackler Sorry about that. Fixed, clippy runs locally, and pushed. |
Thanks! I will cut a release tomorrow. |
Allows for derivation of FromSql and ToSql for generic types. Borrowed types are allowed for ToSql, but not meaningful to support for FromSql (so not supported). (also noted in #570)
Would you be interested in this change? A test was added. Happy to iterate.