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

feat: Script.invoke_async #206

Merged
merged 3 commits into from
May 17, 2019
Merged

feat: Script.invoke_async #206

merged 3 commits into from
May 17, 2019

Conversation

emschwartz
Copy link
Contributor

@emschwartz emschwartz commented Apr 20, 2019

resolves #175

resolves redis-rs#175
also changes Script and ScriptInvocation to use an Arc so that the value lasts for the execution of the future
@emschwartz
Copy link
Contributor Author

@mitsuhiko @badboy @ayosec any feedback on this? If it looks okay, would you be open to merging it? Thanks!

@badboy
Copy link
Collaborator

badboy commented May 2, 2019

I'm currently busy and will only get to it in a week or 2.

src/script.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@Marwes Marwes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@emschwartz
Copy link
Contributor Author

@Marwes Do you have permission to merge this into master? Is there anyone else that needs to take a look at it before it's merged in? Thanks!

@Marwes
Copy link
Collaborator

Marwes commented May 17, 2019

@emschwartz I can merge it, but I can't do a release so I figure I may as well wait for @badboy to approve. If you need to use it now you would need to point it a git dependency anyway so.

@badboy
Copy link
Collaborator

badboy commented May 17, 2019

If @Marwes took a look at the code and approved we can merge it!

We should get a release out the door soon. My next week clears up a bit so I hope to push it out by then.

@badboy badboy merged commit 6945eec into redis-rs:master May 17, 2019
@badboy
Copy link
Collaborator

badboy commented May 17, 2019

Oh btw, feel free to send a PR adding it to the changelog. Otherwise I'm happy to fold that in before I cut a release.

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.

Mixing sync and async connections results in Connection refused (os error 111)
3 participants