Code review fixes

Signed-off-by: Theo Despoudis <thdespou@hotmail.com>

3 files changed, 61 insertions, 58 deletions

diff --git a/redis/client.py b/redis/client.py
index 644165c..3cf37be 100755
--- a/redis/client.py
+++ b/redis/client.py
@@ -413,6 +413,12 @@ def parse_pubsub_numsub(response, **options):
     return list(zip(response[0::2], response[1::2]))
 
 
+def parse_client_kill(response, **options):
+    if isinstance(response, (long, int)):
+        return int(response)
+    return nativestr(response) == 'OK'
+
+
 class Redis(object):
     """
     Implementation of the Redis protocol.
@@ -471,7 +477,7 @@ class Redis(object):
         {
             'CLIENT GETNAME': lambda r: r and nativestr(r),
             'CLIENT ID': int,
-            'CLIENT KILL': lambda r: int or nativestr(r) == 'OK',
+            'CLIENT KILL': parse_client_kill,
             'CLIENT LIST': parse_client_list,
             'CLIENT SETNAME': bool_ok,
             'CLIENT UNBLOCK': lambda r: r and int(r) == 1 or False,
@@ -790,37 +796,40 @@ class Redis(object):
         "Disconnects the client at ``address`` (ip:port)"
         return self.execute_command('CLIENT KILL', address)
 
-    def client_kill_filter(self, *filter_options):
+    def client_kill_filter(self, _id=None, _type=None, addr=None, skipme=None):
         """
         Disconnects the client using a variety of filter options
-        :param filter_options: a tuple or list of filter options with
-        the following format:
-        (filter, value, filter, value,...) or
-        [filter, value, filter, value,...]
-        """
-        if not isinstance(filter_options, (list, tuple)) or not filter_options:
+        :param _id: Allows to kill a client by its unique ID field
+        :param _type: Kill by type. Where type is one of normal,
+        master, slave and pubsub
+        :param addr: This is exactly the same as the client_kill
+        :param skipme: If yes, then he client calling the command
+        will not get killed.
+        No will have the effect of also killing the client calling the command.
+        """
+        filter_options = ()
+        if _type is not None:
+            client_types = ('normal', 'master', 'slave', 'pubsub')
+            if str(_type).lower() not in client_types:
+                raise DataError("CLIENT KILL type must be one of %r" % (
+                                client_types,))
+            filter_options = filter_options + (Token.get_token('TYPE'), _type)
+        if skipme is not None:
+            yes_no = ('yes', 'no')
+            if str(skipme).lower() not in yes_no:
+                raise DataError("CLIENT KILL skipme yes/no must be one of %r"
+                                % (yes_no,))
+            filter_options = filter_options + (Token.get_token('SKIPME'),
+                                               skipme)
+        if _id is not None:
+            filter_options = filter_options + (Token.get_token('ID'),
+                                               _id)
+        if addr is not None:
+            filter_options = filter_options + (Token.get_token('ADDR'),
+                                               addr)
+        if len(filter_options) == 0:
             raise DataError("CLIENT KILL <filter> <value> ... ... <filter> "
-                            "<value> "
-                            "must be a non empty list or "
-                            "tuple to execute")
-        if len(filter_options) % 2 != 0:
-            raise DataError("CLIENT KILL <filter> <value> requires a filter "
-                            "and a value pair. Got %r" % (filter_options,))
-        filter_types = ('addr', 'id', 'type', 'skipme')
-        client_types = ('normal', 'master', 'slave', 'pubsub')
-        yes_no = ('yes', 'no')
-        for index in range(0, len(filter_options), 2):
-            key = str(filter_options[index]).lower()
-            value = str(filter_options[index + 1]).lower()
-            if key not in filter_types:
-                raise DataError("CLIENT KILL <filter> must be one of %r" % (
-                    filter_types,))
-            if key == 'type' and value not in client_types:
-                raise DataError("CLIENT KILL TYPE <value> "
-                                "must be one of %r" % (client_types,))
-            if key == 'skipme' and value not in yes_no:
-                raise DataError("CLIENT KILL SKIPME <value> "
-                                "must be one of %r" % (yes_no,))
+                            "<value> must specify at least one filter")
         return self.execute_command('CLIENT KILL', *filter_options)
 
     def client_list(self, _type=None):
diff --git a/tests/conftest.py b/tests/conftest.py
index 7a36954..6e046f2 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -48,11 +48,10 @@ def r(request, **kwargs):
 
 
 @pytest.fixture()
-def r3(request, **kwargs):
+def r2(request, **kwargs):
     return [
         _get_client(redis.Redis, request, **kwargs),
         _get_client(redis.Redis, request, **kwargs),
-        _get_client(redis.Redis, request, **kwargs)
     ]
 
 
diff --git a/tests/test_commands.py b/tests/test_commands.py
index b6177f7..986345b 100644
--- a/tests/test_commands.py
+++ b/tests/test_commands.py
@@ -95,42 +95,37 @@ class TestRedisCommands(object):
         assert r.client_setname('redis_py_test')
         assert r.client_getname() == 'redis_py_test'
 
-    @skip_if_server_version_lt('2.4.0')
-    def test_client_kill(self, r, r3):
-        clients = r.client_list()
-        r.client_kill(clients[1].get('addr'))
-        clients = r.client_list()
-        assert len(clients) == 3
+    @skip_if_server_version_lt('2.6.9')
+    def test_client_kill(self, r, r2):
+        r.client_setname('redis-py-c1')
+        r2[0].client_setname('redis-py-c2')
+        r2[1].client_setname('redis-py-c3')
+        test_clients = [client for client in r.client_list()
+                        if client.get('name')
+                        in ['redis-py-c1', 'redis-py-c2', 'redis-py-c3']]
+        assert len(test_clients) == 3
+
+        resp = r.client_kill(test_clients[1].get('addr'))
+        assert isinstance(resp, bool) and resp is True
+
+        test_clients = [client for client in r.client_list()
+                        if client.get('name')
+                        in ['redis-py-c1', 'redis-py-c2', 'redis-py-c3']]
+        assert len(test_clients) == 2
 
     @skip_if_server_version_lt('2.8.12')
     def test_client_kill_filter_invalid_params(self, r):
-        # invalid type
-        with pytest.raises(exceptions.DataError):
-            r.client_kill_filter(123)
-
-        # empty list
-        with pytest.raises(exceptions.DataError):
-            r.client_kill_filter([])
-
-        # empty tuple
-        with pytest.raises(exceptions.DataError):
-            r.client_kill_filter(())
-
-        # missing values
+        # empty
         with pytest.raises(exceptions.DataError):
-            r.client_kill_filter(*["type", "master", "skipme"])
+            r.client_kill_filter()
 
-        # invalid filter
+        # invalid skipme
         with pytest.raises(exceptions.DataError):
-            r.client_kill_filter(*["type", "master", "allow", "yes"])
+            r.client_kill_filter(skipme="yeah")
 
         # invalid type
         with pytest.raises(exceptions.DataError):
-            r.client_kill_filter(*["type", "caster", "skipme", "yes"])
-
-        # invalid skipme
-        with pytest.raises(exceptions.DataError):
-            r.client_kill_filter(*["type", "master", "skipme", "yeah"])
+            r.client_kill_filter(_type="caster")
 
     @skip_if_server_version_lt('2.6.9')
     def test_client_list_after_client_setname(self, r):