Added the MOVE command to finagle-redis. #267
Conversation
Finagle-redis does not implement all valid redis commands. Before embarking to flesh out remaining commands, I want to verify that any commands added are done in the correct testing and stylistic manner. The MOVE command has been added as a Key command
| @@ -65,6 +65,21 @@ trait Keys { self: BaseClient => | |||
| } | |||
|
|
|||
| /** | |||
| * Move key from the currently selected database to the specified destination | |||
|
Thanks for the contribution! Could you also add a test to ClientSpec and ClientServerIntegrationSpec? |
|
How did you test with 2.11, by the way? I don't think we've published a good 2.11 yet. Did you do a publish-local on util? |
|
Yeah, I didn't test against 2.11.0, my apologies for stating that. When I was in Build.scala I had set this crossScalaVersions := Seq("2.9.2", "2.10.0", "2.11.0")In anticipation of testing 2.11.0 but never got to it. I had it written down in my notes but hadn't gone back to correct it. |
The scaladoc formatting for the Redis MOVE command wasn't appropriate, and additional formatting changes in the method itself were required. Scaladoc formatting has been fixed along with code style issues.
|
Just notice this while in Build.scala testOptions in Test := Seq(Tests.Filter {
case "com.twitter.finagle.redis.protocol.integration.ClientServerIntegrationSpec" => true //false
case "com.twitter.finagle.redis.integration.ClientSpec" => true // false
case "com.twitter.finagle.redis.integration.BtreeClientSpec" => false
case "com.twitter.finagle.redis.integration.ClientServerIntegrationSpec" => true // false
case _ => true
})That first Spec sbt test-only com.twitter.finagle.redis.protocol.integration.ClientServerIntegrationSpecwas giving me zilch. |
|
I think so, not sure what happened with that. |
There was a stale reference to an old Spec inside the finagle-redis portion of Build.scala. This was causing confusion when trying to invoke ClientServerIntegrationSpec, as we had similarly named classes in different packages. The stale reference to ClientServerIntegrationSpec has been removed.
Additional testing was requested for the finagle-redis MOVE command impl. Added tests to the ClientSpec and the ClientServerSpec.
|
This looks good, let me get another person to take a look too. Also, just a heads-up: if you're going to do a ton of redis commands, it will probably make sense to break it up into a bunch of different PRs so we can review as you go along. |
|
lgtm! |
|
This has been merged internally, it should show up in the github repo sometime next week. Thanks for the contribution! |
|
It's on github! |
Finagle-redis does not implement all valid redis commands. Before embarking to flesh out
remaining commands, I want to verify that any commands added are done in the correct
testing and stylistic manner.
The MOVE command has been added as a Key command
Tested with Scala 2.9.2, 2.10.3, 2.11.0
Tested with Redis 2.8