Skip to content
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

Expose build_transaction through AsyncConnectionWrapper #116

Closed

Conversation

banool
Copy link

@banool banool commented Sep 30, 2023

This addresses #115.

I feel I've made the section in async_connection_wrapper more complex than necessary, I can try tidy it up.

The tests don't pass right now but I know the core section compiles.

Let me know what you think about this solution!

@banool banool force-pushed the banool/build_transaction_wrapper branch from 5145d38 to 5d2f5e8 Compare September 30, 2023 12:15
@banool banool marked this pull request as ready for review September 30, 2023 12:16
@banool
Copy link
Author

banool commented Sep 30, 2023

Another approach would be to add a function that lets the user call functions inner.

@weiznich
Copy link
Owner

weiznich commented Oct 1, 2023

Thanks for opening this PR. It addresses the problem that there is no build_transaction function for the async connection wrapper type, but I don't feel that the implemented solution is particular help full for down stream crates using that type because the indirectly exposed run function is still async. I would prefer a solution that exposes a sync run method as well. That migth require duplicating or at least modifying parts of TransactionBuilder type, by having a separate run function depending on whether the inner connection type implements diesel::Connection or diesel_async::AsyncConnection.

@banool
Copy link
Author

banool commented Oct 1, 2023

Hmmmm yeah good point. In my case I'm actually already in an async environment so this would work for me, but I realize that that's not the intent of the AsyncConnectionWrapper (since in my case I'd have to use spawn_blocking everywhere). I think I'll just try switch to using diesel-async proper, so I'm going to just close this PR for now. I'll leave the issue open though.

@banool banool closed this Oct 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants