Skip to content

Commit

Permalink
Addressing corner cases for datastore.Key.__eq__.
Browse files Browse the repository at this point in the history
These corners are a direct result of #274.
  • Loading branch information
dhermes committed Oct 23, 2014
1 parent 3d4ce5d commit b61ba2e
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 3 deletions.
21 changes: 18 additions & 3 deletions gcloud/datastore/key.py
Original file line number Diff line number Diff line change
Expand Up @@ -256,9 +256,24 @@ def __eq__(self, other):
if self is other:
return True

return (self._dataset_id == other._dataset_id and
self.namespace() == other.namespace() and
self.path() == other.path())
if not isinstance(other, self.__class__):
return False

# Check that paths match.
if self.path() != other.path():
return False

# Check that datasets match.
if not (self._dataset_id == other._dataset_id or
self._dataset_id is None or other._dataset_id is None):

This comment has been minimized.

Copy link
@tseaver

tseaver Oct 23, 2014

Contributor

Why special casing for None? If both are None, shouldn't the keys be equivalent?

This comment has been minimized.

Copy link
@dhermes

dhermes Oct 23, 2014

Author Contributor

It is special casing for the unset case, which is now a possibility for retrieved entities due to #247

return False

# Check that namespaces match.
if not (self._namespace == other._namespace or
self._namespace is None or other._namespace is None):

This comment has been minimized.

Copy link
@tseaver

tseaver Oct 23, 2014

Contributor

Even more true for namespaces -- most keys will never have one assigned, AFAICT.

This comment has been minimized.

Copy link
@dhermes

dhermes Oct 23, 2014

Author Contributor

That's certainly not true. Large applications will use namespaces by default to silo data. I'm unclear if the long tail dominates the large users but it's certainly not a trivial amount.

Anyhow, the namespace of u'' is set when retrieved from a query for an entity with no namespace. However, our default behavior on keys created locally is to set it to None. We could strengthen this to checking that they are both False-y.

FWIW I'm happy this discussion is taking place and hope it leads to simpler checks here and more consistent behavior across our classes.

return False

return True

def __ne__(self, other):
return not self.__eq__(other)
18 changes: 18 additions & 0 deletions gcloud/datastore/test_key.py
Original file line number Diff line number Diff line change
Expand Up @@ -332,3 +332,21 @@ def test_key___eq__(self):
self.assertEqual(key1, key1)
key3 = self._getTargetClass().from_path('abc', 'ghi')
self.assertNotEqual(key1, key3)

def test_key___eq___wrong_type(self):
key = self._getTargetClass().from_path('abc', 'def')
self.assertNotEqual(key, 10)

def test_key___eq___dataset_id(self):
key1 = self._getTargetClass().from_path('abc', 'def')
key2 = self._getTargetClass().from_path('abc', 'def', dataset_id='foo')
self.assertEqual(key1, key2)
key3 = self._getTargetClass().from_path('abc', 'def', dataset_id='bar')
self.assertNotEqual(key2, key3)

def test_key___eq___namespace(self):
key1 = self._getTargetClass().from_path('abc', 'def')
key2 = self._getTargetClass().from_path('abc', 'def', namespace='foo')
self.assertEqual(key1, key2)
key3 = self._getTargetClass().from_path('abc', 'def', namespace='bar')
self.assertNotEqual(key2, key3)

0 comments on commit b61ba2e

Please sign in to comment.