Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Updated the set method to allow options as per redis-rb 3.0.6. #80

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
Contributor

djnawara commented Dec 4, 2013

pair: @ebenoist

This should resolve issue #79

We left the method a little fat, as we weren't sure what kind of method abstraction you prefer in the code base.

Let us know if you have any questions.

Cheers,
Dave & Erik

Contributor

ebenoist commented Dec 23, 2013

@djnawara Let's rebase this tomorrow if you have a second. I think it'll get the build to pass. @guilleiguaran do you have any opinion on these changes? We're working off of @djnawara fork at the moment, but we'd love some input (and eventually to get back on your master branch).

Contributor

larrylv commented Dec 23, 2013

I'm not a fan of this implementation.

The reason why fakeredis doesn't support options is: in redis-rb, options are parsed like this, so in fakeredis' set method, options becomes an array.

I think we should let fakeredis' interfaces being the same with redis-rb. So my suggestion is monkey-patch Redis like what I did in another PR #83, codes are here. It just changes the way Redis calls client.

What do you guys think?

Contributor

ebenoist commented Dec 30, 2013

@djnawara the 1.8.7 and ree build is still failing, I imagine its the hash syntax.

Collaborator

caius commented Jan 27, 2014

Why not leave Redis alone and just turn options back into a hash in the fakeredis methods? We could extract this to a helper method if more than one method finds it useful too.

Something like

def sort(key, value, *options)
  options = Hash[options.each_slice(2).to_a]

would turn the array back into a hash for instance. I'd rather we weren't monkeypatching other things outside our direct control unless we absolutely have to. 😄

Contributor

djnawara commented Jan 28, 2014

Hey all…

I updated the request for 1.8.7 compatibility and played around with this a bit.

I had to find a middle ground, since the redis client mucks with the options a bit too much and doesn't add the value for NX or XX in the args array. Because of this, you end up with a hash where "NX" => "XX". I just deleted those out of the array options before converting to a hash; converting to a hash does clean up the expiration options processing a bit.

Owner

guilleiguaran commented Feb 6, 2014

@caius wdyt about new implentation of @djnawara ? 😄

@caius caius commented on the diff May 20, 2014

spec/keys_spec.rb
@@ -265,5 +265,61 @@ module FakeRedis
@client.select(0)
@client.get("key1").should be == "1"
end
+
+ context "with extended options" do
+ it "uses ex option to set the expire time, in seconds" do
+ now = Time.now
+ Time.stub({ :now => now })
+ ttl = 7
+
+ @client.set("key1", "1", { :ex => ttl }).should == "OK"
+ @client.client.connection.find_database.expires["key1"].should == now + ttl
@caius

caius May 20, 2014

Collaborator

Can this be changed to @client.ttl.should == ttl instead please? This blows up if we run this test suite against an actual redis server using bundle exec rspec -r spec_helper_live_redis.rb spec, when it has no need to from what I can see. We should try and use redis commands to inspect the data where we can!

Collaborator

caius commented May 20, 2014

@djnawara can you merge master into this PR please?

And I also think you can use redis' ttl command to inspect the TTLs on keys, rather than reaching into fakeredis' data structures directly, which are both liable to change in future, and doesn't work when the test suite is run against a live redis server to make sure our specs are accurate.

The following patch makes the test suite pass both against fakeredis and an actual redis server for me:

diff --git i/spec/keys_spec.rb w/spec/keys_spec.rb
index 8172271..52b1f05 100644
--- i/spec/keys_spec.rb
+++ w/spec/keys_spec.rb
@@ -268,36 +268,30 @@ module FakeRedis

     context "with extended options" do
       it "uses ex option to set the expire time, in seconds" do
-        now = Time.now
-        Time.stub({ :now => now })
         ttl = 7

         @client.set("key1", "1", { :ex => ttl }).should == "OK"
-        @client.client.connection.find_database.expires["key1"].should == now + ttl
+        @client.ttl("key1").should == ttl
       end

       it "uses px option to set the expire time, in miliseconds" do
-        now = Time.now
-        Time.stub({ :now => now })
         ttl = 7000

         @client.set("key1", "1", { :px => ttl }).should == "OK"
-        @client.client.connection.find_database.expires["key1"].should == now + (ttl / 1000)
+        @client.ttl("key1").should == (ttl / 1000)
       end

       # Note that the redis-rb implementation will always give PX last.
       # Redis seems to process each expiration option and the last one wins.
       it "prefers the finer-grained PX expiration option over EX" do
-        now = Time.now
-        Time.stub({ :now => now })
-        ttl_px = 5555
+        ttl_px = 6000
         ttl_ex = 10

         @client.set("key1", "1", { :px => ttl_px, :ex => ttl_ex })
-        @client.client.connection.find_database.expires["key1"].should == now + (ttl_px / 1000.0)
+        @client.ttl("key1").should == (ttl_px / 1000)

         @client.set("key1", "1", { :ex => ttl_ex, :px => ttl_px })
-        @client.client.connection.find_database.expires["key1"].should == now + (ttl_px / 1000.0)
+        @client.ttl("key1").should == (ttl_px / 1000)
       end

       it "uses nx option to only set the key if it does not already exist" do

Thanks!

Collaborator

caius commented May 20, 2014

Closing this out in favour of #106

@caius caius closed this May 20, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment