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

add usage of scoped futures to transactions: ownership of this change transferred to @weiznich #38

Merged
merged 9 commits into from
Nov 12, 2022

Conversation

treysidechain
Copy link
Contributor

@treysidechain treysidechain commented Nov 8, 2022

Hi there! I've been building on top of the diesel-async library for some time and have wanted to be able to use references in the closures I pass to AsyncConnection's transaction method. I wrote up a small crate that encapsulates the behavior needed to support references in the closure, basically it imposes an upper bound on the higher-ranked lifetime that's implicit in the FnOnce trait bound so that it doesn't imply a 'static bound.

Main downside of merging this would be pushing a new function scope_boxed onto code calling transaction, however, Box::pinned still works with the proposed changes.

Here is an example showing how the updated transaction signature allows callback closures to capture references.

@weiznich
Copy link
Owner

Thanks for opening this PR, this looks great 👍
I would like to reexport the ScopedFutureExt trait from diesel_async so that it is easier for users to import this trait.

In addition: I potentially plan to relicense diesel-async in the future. Because of that I need an explicitly written consent that you transfer the ownership of this change to me. Please add that to the commit message.

@treysidechain treysidechain changed the title add usage of scoped futures to transactions add usage of scoped futures to transactions: ownership of this change transferred to @weiznich Nov 11, 2022
@treysidechain
Copy link
Contributor Author

sure thing, I updated the PR title. If that's not what you're looking for let me know

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.

2 participants