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

Support *SCAN methods on async connections #326

Merged
merged 9 commits into from
Jun 1, 2020
Merged

Conversation

swilcox3
Copy link
Contributor

Following the pattern set by Iter, I implemented an AsyncIter and exposed it up through AsyncCommands. Thanks to @Marwes who helped me a lot.
Unfortunately I'm on Windows, so I don't have redis-server installed and I can't run the tests myself. I usually use Docker to run redis.

@swilcox3 swilcox3 requested a review from Marwes May 30, 2020 19:25
@swilcox3
Copy link
Contributor Author

Oh, this doesn't work because the implementations of aio::ConnectionLike are calling get_packed_command, not get_packed_command_with_cursor. I'll fix that.

@swilcox3
Copy link
Contributor Author

OK fixed. It's a breaking change though because I had to change the signature of aio::ConnectionLike to match the signature of ConnectionLike, replacing the Cmd with &[u8].

@swilcox3
Copy link
Contributor Author

Finally, got the tests to pass. Man, it's crippling when you can't run them all. The only failure now is clippy saying that we should name the next() method something else because it resembles Iterator. I like that it resembles the Stream syntax, but it could definitely be confusing. Should we rename the function or allow the warning?

@Marwes
Copy link
Collaborator

Marwes commented May 31, 2020

I'd rename it to next_item or something then.

I guess you could get tests working locally if you tweak the tests to read an environment variable which contains a custom command to run instead of redis and use that start redis in a docker instance,

@swilcox3 swilcox3 requested a review from Marwes June 1, 2020 10:57
@Marwes Marwes merged commit f888db0 into redis-rs:master Jun 1, 2020
@@ -62,7 +62,7 @@ cluster = ["crc16", "rand"]
script = ["sha1"]
async-std-comp = ["aio", "async-std"]
tokio-comp = ["aio", "tokio"]
connection-manager = ["tokio-rt-core", "arc-swap", "futures"]
connection-manager = ["futures", "tokio-rt-core", "arc-swap", "futures"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this duplicate value somehow necessary? Curious as I'm working on a merge into #319. I think this can be trimmed.

Thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, that can probably be removed.

Copy link
Contributor

@Terkwood Terkwood Jun 2, 2020

Choose a reason for hiding this comment

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

Cool, I've updated #319. Cheers.

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.

3 participants