-
-
Notifications
You must be signed in to change notification settings - Fork 1.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
Specify type of diesel::row::NamedRow::get in QueryableByName macro. #4132
Specify type of diesel::row::NamedRow::get in QueryableByName macro. #4132
Conversation
be82672
to
30a5d33
Compare
This looks like an issue, I'll look into it a bit more:
|
That's related to the rust release yesterday. See https://github.com/diesel-rs/diesel/pull/4130/files for the fixes for CI. Otherwise I would appreciate an added test for this case. |
This has the signature ``` fn get<ST, T>(&self, column_name: &str) -> Result<T> where T: FromSql<ST, DB> ``` This can be inferred from the resulting value, if there is only one `ST` type in scope for `FromSql<ST, DB>`. But if multiple types are in scope, e.g. ``` impl<__DB: diesel::backend::Backend> QueryableByName<__DB> for StructWithTextAndCitext where String: diesel::deserialize::FromSql<sql_types::Text, __DB>, String: diesel::deserialize::FromSql<sql_types::Citext, __DB>, { ``` then `ST` cannot be inferred.
a5b305e
to
049fb23
Compare
These tests are still failing for the correctly reported reason. The shorter error messages seem to be because this change reduces the inference that's happening; this reduces some noise when the compilation fails for other reasons.
4ab8a66
to
2bcdd5d
Compare
…for one output type. This tests that `QueryableByName` compiles, and outputs some data, for one example where the types are pulled from a `table!` macro; and another example where the values are specified as annotations in the struct. It's difficult to find a type that has the required properties across all three databases; these tests are compiled only for Postgres. To do something similar for the other databases would probably require compiling with one of the time libraries.
2bcdd5d
to
99546fd
Compare
@weiznich Added two tests. They 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.
Thanks 👍
I changed the test case to not depend on citext anymore. This solves the parallel initialization issue. I verified that it still produces the issue without fix.
6940a68
to
8ec670b
Compare
8ec670b
to
302fcd2
Compare
Thanks! |
…le_db_types Specify type of diesel::row::NamedRow::get in QueryableByName macro.
…le_db_types Specify type of diesel::row::NamedRow::get in QueryableByName macro.
Resolves #4131
diesel::row::NamedRow::get
has the signatureThis can be inferred from the resulting value, if there is only one
ST
type in scope forFromSql<ST, DB>
.But if multiple types are in scope, e.g.
then
ST
cannot be inferred.