Repository: civisanalytics/civis-python

Issue:
`civis.find` has unintuitive behavior with boolean kwargs
The `civis.find` (and `civis.find_one`) function allows users to filter a list of `Response` objects for user-supplied attribute/value pairs. However, the behavior when given a keyword argument with a boolean value is different than when given a keyword argument with any other value. If for example, one calls `civis.find(objs, spam='eggs')`, then the returned list contains all objects which have an attribute "spam" with value "eggs". However, `civis.find(objs, spam=True)` will return all objects which have an attribute "spam", regardless of value.

Perhaps it's worthwhile to have a way to filter on presence or absence of an attribute, but this means that there's no way to filter objects based on attributes with boolean values. I suggest that we treat booleans the same way as any other value type.

Regardless of whether we treat booleans the same way as other values, there's also a bug in the way we handle them now. If a user supplies a keyword argument with a value of `False`, then the returned list will always be empty, no matter what's in the objects.

I noticed this issue when reviewing #224 .

Discussion:

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 5dcc23a..6ea90be 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -22,6 +22,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
   Also dropped the deprecated methods in `ServiceClient`. (#471)
 - The `return_type` parameter of a `civis.response.Response` object
   no longer has the `"pandas"` option. (#472)
+- When `civis.find` uses kwargs as filters, boolean values are now treated in the same
+  way as other data types for value equality comparison, rather than the presence or
+  absence of the key in question. (#473)
 
 ### Added
 - Added error handling of file_id with type string passed to `civis.io.civis_file_to_table`. (#454)
diff --git a/civis/civis.py b/civis/civis.py
index e05a5e3..3c3fd42 100644
--- a/civis/civis.py
+++ b/civis/civis.py
@@ -29,15 +29,16 @@ def find(object_list, filter_func=None, **kwargs):
         if and only if ``bool(filter_func(object))`` is ``True``.
     **kwargs
         Key-value pairs for more fine-grained filtering; they cannot be used
-        in conjunction with `filter_func`. All keys must be strings.
-        For an `object` from the input iterable to be included in the
-        returned list, all the `key`s must be attributes of `object`, plus
-        any one of the following conditions for a given `key`:
+        in conjunction with ``filter_func``. All keys must be strings.
+        For an object ``obj`` from the input iterable to be included in the
+        returned list, all the keys must be attributes of ``obj``, plus
+        any one of the following conditions for a given key:
 
-        - `value` is a one-argument function and
-          ``bool(value(getattr(object, key)))`` is ``True``
-        - `value` is ``True``
-        - ``getattr(object, key)`` is equal to ``value``
+        - ``value`` is a one-argument function and
+          ``bool(value(getattr(obj, key)))`` is equal to ``True``
+        - ``value`` is either ``True`` or ``False``, and
+          ``getattr(obj, key) is value`` is ``True``
+        - ``getattr(obj, key) == value`` is ``True``
 
     Returns
     -------
@@ -67,7 +68,7 @@ def find(object_list, filter_func=None, **kwargs):
                     if not v(getattr(o, k, None)):
                         return False
                 elif isinstance(v, bool):
-                    if hasattr(o, k) != v:
+                    if getattr(o, k) is not v:
                         return False
                 elif v != getattr(o, k, None):
                     return False


Test patch:
diff --git a/civis/tests/test_civis.py b/civis/tests/test_civis.py
index 8dd543e..c1cc0dd 100644
--- a/civis/tests/test_civis.py
+++ b/civis/tests/test_civis.py
@@ -1,11 +1,37 @@
 from unittest import mock
 
 import civis
+from civis import find
 from civis.resources import API_SPEC
+from civis.response import Response
 
 import pytest
 
 
+def test_find_filter_with_kwargs():
+    r1 = Response({"foo": 0, "bar": "a", "baz": True})
+    r2 = Response({"foo": 1, "bar": "b", "baz": True})
+    r3 = Response({"foo": 2, "bar": "b", "baz": False})
+
+    assert find([r1, r2, r3], wrong_attr="whatever") == []
+
+    assert find([r1, r2, r3], foo=0) == [r1]
+    assert find([r1, r2, r3], foo=1) == [r2]
+    assert find([r1, r2, r3], foo=1, bar="b") == [r2]
+
+    assert find([r1, r2, r3], bar="b") == [r2, r3]
+    assert find([r1, r2, r3], bar="b", foo=1) == [r2]
+
+    assert find([r1, r2, r3], foo=True) == []
+    assert find([r1, r2, r3], foo=False) == []
+    assert find([r1, r2, r3], bar=True) == []
+    assert find([r1, r2, r3], bar=False) == []
+    assert find([r1, r2, r3], baz=True) == [r1, r2]
+    assert find([r1, r2, r3], baz=False) == [r3]
+
+    assert find([r1, r2, r3], foo=int) == [r2, r3]
+
+
 @pytest.mark.parametrize('schema_tablename', [
     'foo.bar', '"foo".bar', 'foo."bar"', '"foo"."bar"'
 ])