add diesel_async attribute to multiconnection macro#5099
Conversation
e7203d0 to
723a3e8
Compare
weiznich
left a comment
weiznich
left a comment
There was a problem hiding this comment.
Thanks for opening this Pr.
Overall that's the right way to go. I have quite a few comments about the details of the implementation. We can move forward with this as soon as they are addressed.
| chrono = [] | ||
| time = [] | ||
| numeric = [] | ||
| async_pool = [] |
There was a problem hiding this comment.
Adding new feature flags to diesel_derives is something we need to avoid at all cost. Feature flags are broken for proc-macro crate, we recently moved away all the code generation from being depended on feature flags. See #5063 for details. A similar approach needs to be taken here, which likely means:
- diesel-async needs to export a conditional expand macro for this feature as
#[doc(hidden)]item - The expanded code needs to use that macro in the generated code to conditionally generate the right code.
| ] | ||
| std = ["serde_json?/std"] | ||
| hashbrown = ["dep:hashbrown"] | ||
| async_pool = ["diesel_derives/async_pool"] |
There was a problem hiding this comment.
This shouldn't be required as well
| } | ||
| impl<'a> diesel::query_builder::BindCollector<'a, MultiBackend> | ||
| for MultiBindCollector<'a> { | ||
| type Buffer = multi_connection_impl::bind_collector::BindValue<'a>; |
There was a problem hiding this comment.
I would rather keep the fully qualified name here, as proc-macro code sometimes is used in wired places, so rather make it as robust as possible.
| { | ||
| let out = { | ||
| let out = multi_connection_impl::bind_collector::BindValue::default(); | ||
| let out = BindValue::default(); |
There was a problem hiding this comment.
Keep the qualified path here as well
| Pg(PgConnection), | ||
| Sqlite(diesel::SqliteConnection), |
There was a problem hiding this comment.
Can we use the real diesel-async connection types here? That's only cosmetic, but helps a bit to judge the generated code.
| let await_token = if is_async { | ||
| quote::quote! { .await } | ||
| } else { | ||
| quote::quote! {} | ||
| }; | ||
|
|
||
| let async_token = if is_async { | ||
| quote::quote! { async } | ||
| } else { | ||
| quote::quote! {} | ||
| }; |
There was a problem hiding this comment.
These should be Option<TokenStream> values
| let load_impl = connection_types.iter().map(|c| { | ||
| let variant_ident = c.name; | ||
| let ty = &c.ty; | ||
| let (impl_execute_returning_count, impl_execute_returning_count_async) = if is_async { |
There was a problem hiding this comment.
This should likely not be a if else with two return values, where each branch only generate one
| let query = SerializedQuery { | ||
| inner: source.as_query(), | ||
| backend: MultiBackend::#variant_ident(Default::default()), | ||
| query_builder: super::query_builder::MultiQueryBuilder::#variant_ident(Default::default()), | ||
| p: std::marker::PhantomData::<#ty>, | ||
| }; |
There was a problem hiding this comment.
Would it be possible to deduplicate this part with the sync counter part below?
There was a problem hiding this comment.
Yes that would be possible. The only difference is the .as_query().
I did not do this, because i do not think the code is better readable that way.
If you disagree I will dedpulicate this.
| let simple_connection = if is_async { | ||
| quote::quote! { diesel_async::SimpleAsyncConnection } | ||
| } else { | ||
| quote::quote! { diesel::connection::SimpleConnection } | ||
| }; | ||
|
|
||
| let transaction_manager = if is_async { | ||
| quote::quote! { diesel_async::TransactionManager } | ||
| } else { | ||
| quote::quote! { diesel::connection::TransactionManager } | ||
| }; |
There was a problem hiding this comment.
That should be part of the helper instead
| }; | ||
|
|
||
| let simple_impls = if is_async { | ||
| if cfg!(feature = "async_pool") { |
There was a problem hiding this comment.
This feature flag needs to be replaced by a macro call in the generated code
062d7f6 to
ea4e454
Compare
ea4e454 to
919c243
Compare
|
Hi, I've modified the commit and hopefully resolved all the issues (except one). |
weiznich
left a comment
weiznich
left a comment
There was a problem hiding this comment.
Looks good now, with the minor fixes applied on top
2 participants
Adds and implements a diesel_async attribute to multiconnection macro.
This change depends on: diesel-rs/diesel_async#295
Prior Discussion: diesel-rs/diesel_async#108
I ported the test pointed out in the discussion in https://github.com/jusax23/diesel_async/tree/dev-async_multi_connection-test
This test in diesel_async is depended on this macro change.
Should I add this test to the existing pr in diesel_async, or open a new one after it is merged?
I added a feature
async_pool, so diesel_async can control the implementation ofPoolableConnection.