Merge pull request #1134 from casperisfine/4.x-deprecations

Add deprecation to help the transition to Redis 5.0

CHANGELOG.md

@@ -1,5 +1,10 @@
 # Unreleased
 
+* Introduce `sadd?` and `srem?` as boolean returning versions of `sadd` and `srem`.
+* Deprecate `sadd` and `srem` returning a boolean when called with a single argument.
+  To enable the redis 5.0 behavior you can set `Redis.sadd_returns_boolean = true`.
+* Deprecate passing `timeout` as a positional argument in blocking commands (`brpop`, `blop`, etc).
+
 # 4.7.1
 
 * Gracefully handle OpenSSL 3.0 EOF Errors (`OpenSSL::SSL::SSLError: SSL_read: unexpected eof while reading`). See #1106

lib/redis.rb

@@ -7,12 +7,13 @@
 class Redis
   BASE_PATH = __dir__
   @exists_returns_integer = true
+  @sadd_returns_boolean = true
 
   Deprecated = Class.new(StandardError)
 
   class << self
     attr_reader :exists_returns_integer
-    attr_accessor :silence_deprecations, :raise_deprecations
+    attr_accessor :silence_deprecations, :raise_deprecations, :sadd_returns_boolean
 
     def exists_returns_integer=(value)
       unless value

lib/redis/commands.rb

@@ -41,10 +41,8 @@ module Commands
     # returning false.
     Boolify = lambda { |value|
       case value
-      when 1
-        true
-      when 0
-        false
+      when Integer
+        value > 0
       else
         value
       end

lib/redis/commands/lists.rb

@@ -254,16 +254,17 @@ def _bpop(cmd, args, &blk)
           options = args.pop
           options[:timeout]
         elsif args.last.respond_to?(:to_int)
-          # Issue deprecation notice in obnoxious mode...
-          args.pop.to_int
+          last_arg = args.pop
+          ::Redis.deprecate!(
+            "Passing the timeout as a positional argument is deprecated, it should be passed as a keyword argument:\n" \
+            "  redis.#{cmd}(#{args.map(&:inspect).join(', ')}, timeout: #{last_arg.to_int})" \
+            "(called from: #{caller(2, 1).first})"
+          )
+          last_arg.to_int
         end
 
         timeout ||= 0
 
-        if args.size > 1
-          # Issue deprecation notice in obnoxious mode...
-        end
-
         keys = args.flatten
 
         command = [cmd, keys, timeout]

lib/redis/commands/sets.rb

@@ -20,15 +20,23 @@ def scard(key)
       #   array of members is specified, holding the number of members that were
       #   successfully added
       def sadd(key, member)
-        send_command([:sadd, key, member]) do |reply|
-          if member.is_a? Array
-            # Variadic: return integer
-            reply
-          else
-            # Single argument: return boolean
-            Boolify.call(reply)
-          end
+        block = if Redis.sadd_returns_boolean && !member.is_a?(Array)
+          ::Redis.deprecate!(
+            "Redis#sadd will always return an Integer in Redis 5.0.0. Use Redis#sadd? instead." \
+            "(called from: #{caller(1, 1).first})"
+          )
+          Boolify
         end
+        send_command([:sadd, key, member], &block)
+      end
+
+      # Add one or more members to a set.
+      #
+      # @param [String] key
+      # @param [String, Array<String>] member one member, or array of members
+      # @return [Boolean] Whether or not at least one member was added.
+      def sadd?(key, member)
+        send_command([:sadd, key, member], &Boolify)
       end
 
       # Remove one or more members from a set.
@@ -40,15 +48,23 @@ def sadd(key, member)
       #   array of members is specified, holding the number of members that were
       #   successfully removed
       def srem(key, member)
-        send_command([:srem, key, member]) do |reply|
-          if member.is_a? Array
-            # Variadic: return integer
-            reply
-          else
-            # Single argument: return boolean
-            Boolify.call(reply)
-          end
+        block = if Redis.sadd_returns_boolean && !member.is_a?(Array)
+          ::Redis.deprecate!(
+            "Redis#sadd will always return an Integer in Redis 5.0.0. Use Redis#sadd? instead." \
+            "(called from: #{caller(1, 1).first})"
+          )
+          Boolify
         end
+        send_command([:srem, key, member], &block)
+      end
+
+      # Remove one or more members from a set.
+      #
+      # @param [String] key
+      # @param [String, Array<String>] member one member, or array of members
+      # @return [Boolean] `Boolean` Whether or not a member was removed.
+      def srem?(key, member)
+        send_command([:srem, key, member], &Boolify)
       end
 
       # Remove and return one or more random member from a set.

lib/redis/distributed.rb

@@ -464,12 +464,13 @@ def _bpop(cmd, args)
         options = args.pop
         options[:timeout]
       elsif args.last.respond_to?(:to_int)
-        # Issue deprecation notice in obnoxious mode...
-        args.pop.to_int
-      end
-
-      if args.size > 1
-        # Issue deprecation notice in obnoxious mode...
+        last_arg = args.pop
+        ::Redis.deprecate!(
+          "Passing the timeout as a positional argument is deprecated, it should be passed as a keyword argument:\n" \
+          "  redis.#{cmd}(#{args.map(&:inspect).join(', ')}, timeout: #{last_arg.to_int})" \
+          "(called from: #{caller(2, 1).first})"
+        )
+        last_arg.to_int
       end
 
       keys = args.flatten
@@ -543,11 +544,21 @@ def sadd(key, member)
       node_for(key).sadd(key, member)
     end
 
+    # Add one or more members to a set.
+    def sadd?(key, member)
+      node_for(key).sadd?(key, member)
+    end
+
     # Remove one or more members from a set.
     def srem(key, member)
       node_for(key).srem(key, member)
     end
 
+    # Remove one or more members from a set.
+    def srem?(key, member)
+      node_for(key).srem?(key, member)
+    end
+
     # Remove and return a random member from a set.
     def spop(key, count = nil)
       node_for(key).spop(key, count)

test/lint/sets.rb

@@ -10,6 +10,14 @@ def test_sadd
       assert_equal ["s1", "s2"], r.smembers("foo").sort
     end
 
+    def test_sadd?
+      assert_equal true, r.sadd?("foo", "s1")
+      assert_equal true, r.sadd?("foo", "s2")
+      assert_equal false, r.sadd?("foo", ["s1", "s2"])
+
+      assert_equal ["s1", "s2"], r.smembers("foo").sort
+    end
+
     def test_variadic_sadd
       target_version "2.3.9" do # 2.4-rc6
         assert_equal 2, r.sadd("foo", ["s1", "s2"])
@@ -29,6 +37,16 @@ def test_srem
       assert_equal ["s2"], r.smembers("foo")
     end
 
+    def test_srem?
+      r.sadd("foo", "s1")
+      r.sadd("foo", "s2")
+
+      assert_equal true, r.srem?("foo", ["s1", "s5"])
+      assert_equal false, r.srem?("foo", ["s3", "s4"])
+
+      assert_equal ["s2"], r.smembers("foo")
+    end
+
     def test_variadic_srem
       target_version "2.3.9" do # 2.4-rc6
         r.sadd("foo", "s1")