From 7c55a047b67575759fb53f15fe27bf8d34a15ad5 Mon Sep 17 00:00:00 2001 From: Tres Seaver Date: Fri, 17 Oct 2014 09:44:56 -0400 Subject: [PATCH 1/3] Avoid interior use of 'key()' setter-getter. Instead, use '_key' directly, adding a '_must_key' property which raises a NoKey exception if None (for use in cases which require a key). Allows removal of the pylint 'maybe-no-member' comments. --- gcloud/datastore/entity.py | 71 ++++++++++++++++++--------------- gcloud/datastore/test_entity.py | 24 +++++++++++ 2 files changed, 62 insertions(+), 33 deletions(-) diff --git a/gcloud/datastore/entity.py b/gcloud/datastore/entity.py index 59948084fa7a..219909f8b166 100644 --- a/gcloud/datastore/entity.py +++ b/gcloud/datastore/entity.py @@ -19,6 +19,10 @@ from gcloud.datastore.key import Key +class NoKey(RuntimeError): + pass + + class Entity(dict): # pylint: disable=too-many-public-methods """:type dataset: :class:`gcloud.datastore.dataset.Dataset` :param dataset: The dataset in which this entity belongs. @@ -82,8 +86,8 @@ def dataset(self): be `None`. It also means that if you change the key on the entity, this will refer to that key's dataset. """ - if self.key(): - return self.key().dataset() + if self._key: + return self._key.dataset() def key(self, key=None): """Get or set the :class:`.datastore.key.Key` on the current entity. @@ -117,8 +121,8 @@ def kind(self): which knows its Kind. """ - if self.key(): - return self.key().kind() + if self._key: + return self._key.kind() @classmethod def from_key(cls, key): @@ -162,6 +166,18 @@ def from_protobuf(cls, pb, dataset=None): # pylint: disable=invalid-name return entity + @property + def _must_key(self): + """Return our key. + + :rtype: :class:`gcloud.datastore.key.Key`. + :returns: our key + :raises: NoKey if key is None + """ + if self._key is None: + raise NoKey('no key') + return self._key + def reload(self): """Reloads the contents of this entity from the datastore. @@ -174,11 +190,8 @@ def reload(self): exists remotely, however it will *not* override any properties that exist only locally. """ - - # Note that you must have a valid key, otherwise this makes no sense. - # pylint: disable=maybe-no-member - entity = self.dataset().get_entity(self.key().to_protobuf()) - # pylint: enable=maybe-no-member + key = self._must_key + entity = key.dataset().get_entity(key.to_protobuf()) if entity: self.update(entity) @@ -190,27 +203,24 @@ def save(self): :rtype: :class:`gcloud.datastore.entity.Entity` :returns: The entity with a possibly updated Key. """ - # pylint: disable=maybe-no-member - key_pb = self.dataset().connection().save_entity( - dataset_id=self.dataset().id(), - key_pb=self.key().to_protobuf(), properties=dict(self)) - # pylint: enable=maybe-no-member + key = self._must_key + dataset = key.dataset() + connection = dataset.connection() + key_pb = connection.save_entity( + dataset_id=dataset.id(), + key_pb=key.to_protobuf(), + properties=dict(self)) # If we are in a transaction and the current entity needs an # automatically assigned ID, tell the transaction where to put that. - transaction = self.dataset().connection().transaction() - # pylint: disable=maybe-no-member - if transaction and self.key().is_partial(): + transaction = connection.transaction() + if transaction and key.is_partial(): transaction.add_auto_id_entity(self) - # pylint: enable=maybe-no-member if isinstance(key_pb, datastore_pb.Key): updated_key = Key.from_protobuf(key_pb) # Update the path (which may have been altered). - # pylint: disable=maybe-no-member - key = self.key().path(updated_key.path()) - # pylint: enable=maybe-no-member - self.key(key) + self._key = key.path(updated_key.path()) return self @@ -222,20 +232,15 @@ def delete(self): set on the entity. Whatever is stored remotely using the key on the entity will be deleted. """ - # NOTE: pylint thinks key() is an Entity, hence key().to_protobuf() - # is not defined. This is because one branch of the return - # in the key() definition returns self. - # pylint: disable=maybe-no-member - self.dataset().connection().delete_entity( - dataset_id=self.dataset().id(), key_pb=self.key().to_protobuf()) - # pylint: enable=maybe-no-member + key = self._must_key + dataset = key.dataset() + dataset.connection().delete_entity( + dataset_id=dataset.id(), key_pb=key.to_protobuf()) def __repr__(self): # An entity should have a key all the time (even if it's partial). - if self.key(): - # pylint: disable=maybe-no-member - return '' % (self.key().path(), + if self._key: + return '' % (self._key.path(), super(Entity, self).__repr__()) - # pylint: enable=maybe-no-member else: return '' % (super(Entity, self).__repr__()) diff --git a/gcloud/datastore/test_entity.py b/gcloud/datastore/test_entity.py index e85b426202af..cd051d47f00b 100644 --- a/gcloud/datastore/test_entity.py +++ b/gcloud/datastore/test_entity.py @@ -85,6 +85,14 @@ def test_from_protobuf(self): self.assertEqual(key.kind(), _KIND) self.assertEqual(key.id(), _ID) + def test_reload_no_key(self): + from gcloud.datastore.entity import NoKey + + dataset = _Dataset() + entity = self._makeOne(None, None) + entity['foo'] = 'Foo' + self.assertRaises(NoKey, entity.reload) + def test_reload_miss(self): dataset = _Dataset() key = _Key(dataset) @@ -105,6 +113,14 @@ def test_reload_hit(self): self.assertTrue(entity.reload() is entity) self.assertEqual(entity['foo'], 'Bar') + def test_save_no_key(self): + from gcloud.datastore.entity import NoKey + + dataset = _Dataset() + entity = self._makeOne(None, None) + entity['foo'] = 'Foo' + self.assertRaises(NoKey, entity.save) + def test_save_wo_transaction_wo_auto_id_wo_returned_key(self): connection = _Connection() dataset = _Dataset(connection) @@ -167,6 +183,14 @@ def test_save_w_returned_key(self): (_DATASET_ID, 'KEY', {'foo': 'Foo'})) self.assertEqual(key._path, [{'kind': _KIND, 'id': _ID}]) + def test_delete_no_key(self): + from gcloud.datastore.entity import NoKey + + dataset = _Dataset() + entity = self._makeOne(None, None) + entity['foo'] = 'Foo' + self.assertRaises(NoKey, entity.delete) + def test_delete(self): connection = _Connection() dataset = _Dataset(connection) From 64c711efea2be83bb282f526b6a385201e42ad41 Mon Sep 17 00:00:00 2001 From: Tres Seaver Date: Fri, 17 Oct 2014 10:15:27 -0400 Subject: [PATCH 2/3] Add tests for logic in Entity.__repr__. Fixes #174. --- gcloud/datastore/entity.py | 3 +-- gcloud/datastore/test_entity.py | 22 ++++++++++++++++++---- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/gcloud/datastore/entity.py b/gcloud/datastore/entity.py index 219909f8b166..59db2e364120 100644 --- a/gcloud/datastore/entity.py +++ b/gcloud/datastore/entity.py @@ -20,7 +20,7 @@ class NoKey(RuntimeError): - pass + """Exception raised by Entity methods which require a key.""" class Entity(dict): # pylint: disable=too-many-public-methods @@ -238,7 +238,6 @@ def delete(self): dataset_id=dataset.id(), key_pb=key.to_protobuf()) def __repr__(self): - # An entity should have a key all the time (even if it's partial). if self._key: return '' % (self._key.path(), super(Entity, self).__repr__()) diff --git a/gcloud/datastore/test_entity.py b/gcloud/datastore/test_entity.py index cd051d47f00b..a3171c2bca36 100644 --- a/gcloud/datastore/test_entity.py +++ b/gcloud/datastore/test_entity.py @@ -88,7 +88,6 @@ def test_from_protobuf(self): def test_reload_no_key(self): from gcloud.datastore.entity import NoKey - dataset = _Dataset() entity = self._makeOne(None, None) entity['foo'] = 'Foo' self.assertRaises(NoKey, entity.reload) @@ -116,7 +115,6 @@ def test_reload_hit(self): def test_save_no_key(self): from gcloud.datastore.entity import NoKey - dataset = _Dataset() entity = self._makeOne(None, None) entity['foo'] = 'Foo' self.assertRaises(NoKey, entity.save) @@ -186,7 +184,6 @@ def test_save_w_returned_key(self): def test_delete_no_key(self): from gcloud.datastore.entity import NoKey - dataset = _Dataset() entity = self._makeOne(None, None) entity['foo'] = 'Foo' self.assertRaises(NoKey, entity.delete) @@ -201,8 +198,23 @@ def test_delete(self): self.assertTrue(entity.delete() is None) self.assertEqual(connection._deleted, (_DATASET_ID, 'KEY')) + def test___repr___no_key_empty(self): + entity = self._makeOne(None, None) + self.assertEqual(repr(entity), '') + + def test___repr___w_key_non_empty(self): + connection = _Connection() + dataset = _Dataset(connection) + key = _Key(dataset) + key.path('/bar/baz') + entity = self._makeOne() + entity.key(key) + entity['foo'] = 'Foo' + self.assertEqual(repr(entity), "") + class _Key(object): + _MARKER = object() _key = 'KEY' _partial = False _path = None @@ -219,7 +231,9 @@ def to_protobuf(self): def is_partial(self): return self._partial - def path(self, path): + def path(self, path=_MARKER): + if path is self._MARKER: + return self._path self._path = path From 961e6954f8666a8a9cb01a1534e039fb9531169d Mon Sep 17 00:00:00 2001 From: Tres Seaver Date: Fri, 17 Oct 2014 13:20:49 -0400 Subject: [PATCH 3/3] Clarify docstring for 'Entity._must_key'. Incorporates feedback from @dhermes. --- gcloud/datastore/entity.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gcloud/datastore/entity.py b/gcloud/datastore/entity.py index 59db2e364120..4f8b877792a8 100644 --- a/gcloud/datastore/entity.py +++ b/gcloud/datastore/entity.py @@ -168,7 +168,7 @@ def from_protobuf(cls, pb, dataset=None): # pylint: disable=invalid-name @property def _must_key(self): - """Return our key. + """Return our key, or raise NoKey if not set. :rtype: :class:`gcloud.datastore.key.Key`. :returns: our key