Fix #260: break import cycles in datastore w/o factories.

tseaver · tseaver · commit edac223348c5 · 2014-10-23T13:36:47.000-04:00
Alternative fix to the 'factories' indirection in #268. Note that this requires adding a messy arg to the 'Entity.from_protobuf' classmethod, in order to avoid the need to import '_helpers'. Another way to avoid this cycle would be to make 'Entity.from_protobuf' into a free function in another module (likely also moving 'Key.from_protobuf' for the sake of consistency).
diff --git a/gcloud/datastore/_helpers.py b/gcloud/datastore/_helpers.py
@@ -123,7 +123,8 @@ def _get_value_from_value_pb(value_pb):
         result = value_pb.blob_value
 
     elif value_pb.HasField('entity_value'):
-        result = Entity.from_protobuf(value_pb.entity_value)
+        result = Entity.from_protobuf(value_pb.entity_value,
+                                      _get_value_from_property_pb)
 
     elif value_pb.list_value:
         result = [_get_value_from_value_pb(x) for x in value_pb.list_value]
diff --git a/gcloud/datastore/dataset.py b/gcloud/datastore/dataset.py
@@ -1,5 +1,10 @@
 """Create / interact with gcloud datastore datasets."""
 
+from gcloud.datastore.entity import Entity
+from gcloud.datastore.query import Query
+from gcloud.datastore.transaction import Transaction
+from gcloud.datastore import _helpers
+
 
 class Dataset(object):
     """A dataset in the Cloud Datastore.
@@ -70,8 +75,6 @@ def query(self, *args, **kwargs):
         :rtype: :class:`gcloud.datastore.query.Query`
         :returns: a new Query instance, bound to this dataset.
         """
-        # This import is here to avoid circular references.
-        from gcloud.datastore.query import Query
         kwargs['dataset'] = self
         return Query(*args, **kwargs)
 
@@ -84,8 +87,6 @@ def entity(self, kind):
         :rtype: :class:`gcloud.datastore.entity.Entity`
         :returns: a new Entity instance, bound to this dataset.
         """
-        # This import is here to avoid circular references.
-        from gcloud.datastore.entity import Entity
         return Entity(dataset=self, kind=kind)
 
     def transaction(self, *args, **kwargs):
@@ -98,8 +99,6 @@ def transaction(self, *args, **kwargs):
         :rtype: :class:`gcloud.datastore.transaction.Transaction`
         :returns: a new Transaction instance, bound to this dataset.
         """
-        # This import is here to avoid circular references.
-        from gcloud.datastore.transaction import Transaction
         kwargs['dataset'] = self
         return Transaction(*args, **kwargs)
 
@@ -125,15 +124,11 @@ def get_entities(self, keys):
         :rtype: list of :class:`gcloud.datastore.entity.Entity`
         :return: The requested entities.
         """
-        # This import is here to avoid circular references.
-        from gcloud.datastore.entity import Entity
-
         entity_pbs = self.connection().lookup(
             dataset_id=self.id(),
             key_pbs=[k.to_protobuf() for k in keys]
         )
 
-        entities = []
-        for entity_pb in entity_pbs:
-            entities.append(Entity.from_protobuf(entity_pb, dataset=self))
-        return entities
+        return [Entity.from_protobuf(
+            entity_pb, _helpers._get_value_from_property_pb,
+            dataset=self) for entity_pb in entity_pbs]
diff --git a/gcloud/datastore/entity.py b/gcloud/datastore/entity.py
@@ -147,7 +147,7 @@ def from_key(cls, key, dataset=None):
         return cls(dataset).key(key)
 
     @classmethod
-    def from_protobuf(cls, pb, dataset=None):
+    def from_protobuf(cls, pb, _get_value, dataset=None):
         """Factory method for creating an entity based on a protobuf.
 
         The protobuf should be one returned from the Cloud Datastore
@@ -156,18 +156,19 @@ def from_protobuf(cls, pb, dataset=None):
         :type pb: :class:`gcloud.datastore.datastore_v1_pb2.Entity`
         :param pb: The Protobuf representing the entity.
 
+        :type _get_value: module
+        :param _get_value: normally, this will be
+               :mod:`gcloud.datastore._helpers._get_value_from_property_pb`,
+               but it could be a shim for testing.
+
         :returns: The :class:`Entity` derived from the
                   :class:`gcloud.datastore.datastore_v1_pb2.Entity`.
         """
-
-        # This is here to avoid circular imports.
-        from gcloud.datastore import _helpers
-
         key = Key.from_protobuf(pb.key)
         entity = cls.from_key(key, dataset)
 
         for property_pb in pb.property:
-            value = _helpers._get_value_from_property_pb(property_pb)
+            value = _get_value(property_pb)
             entity[property_pb.name] = value
 
         return entity
diff --git a/gcloud/datastore/query.py b/gcloud/datastore/query.py
@@ -343,8 +343,9 @@ def fetch(self, limit=None):
         entity_pbs, end_cursor = query_results[:2]
 
         self._cursor = end_cursor
-        return [Entity.from_protobuf(entity, dataset=self.dataset())
-                for entity in entity_pbs]
+        return [Entity.from_protobuf(
+            entity, _helpers._get_value_from_property_pb,
+            dataset=self.dataset()) for entity in entity_pbs]
 
     def cursor(self):
         """Returns cursor ID
diff --git a/gcloud/datastore/test_entity.py b/gcloud/datastore/test_entity.py
@@ -78,6 +78,7 @@ def test_from_key_w_dataset(self):
 
     def test_from_protobuf_wo_dataset(self):
         from gcloud.datastore import datastore_v1_pb2 as datastore_pb
+        from gcloud.datastore._helpers import _get_value_from_property_pb
 
         entity_pb = datastore_pb.Entity()
         entity_pb.key.partition_id.dataset_id = _DATASET_ID
@@ -87,7 +88,7 @@ def test_from_protobuf_wo_dataset(self):
         prop_pb.name = 'foo'
         prop_pb.value.string_value = 'Foo'
         klass = self._getTargetClass()
-        entity = klass.from_protobuf(entity_pb)
+        entity = klass.from_protobuf(entity_pb, _get_value_from_property_pb)
         self.assertTrue(entity.dataset() is None)
         self.assertEqual(entity.kind(), _KIND)
         self.assertEqual(entity['foo'], 'Foo')
@@ -99,6 +100,7 @@ def test_from_protobuf_wo_dataset(self):
     def test_from_protobuf_w_dataset(self):
         from gcloud.datastore import datastore_v1_pb2 as datastore_pb
         from gcloud.datastore.dataset import Dataset
+        from gcloud.datastore._helpers import _get_value_from_property_pb
 
         entity_pb = datastore_pb.Entity()
         entity_pb.key.partition_id.dataset_id = _DATASET_ID
@@ -109,7 +111,8 @@ def test_from_protobuf_w_dataset(self):
         prop_pb.value.string_value = 'Foo'
         dataset = Dataset(_DATASET_ID)
         klass = self._getTargetClass()
-        entity = klass.from_protobuf(entity_pb, dataset)
+        entity = klass.from_protobuf(
+            entity_pb, _get_value_from_property_pb, dataset)
         self.assertTrue(entity.dataset() is dataset)
         self.assertEqual(entity.kind(), _KIND)
         self.assertEqual(entity['foo'], 'Foo')
diff --git a/pylintrc_default b/pylintrc_default
@@ -24,7 +24,7 @@ ignore = datastore_v1_pb2.py
 [MESSAGES CONTROL]
 disable = I, protected-access, maybe-no-member, no-member,
           redefined-builtin, star-args, missing-format-attribute,
-          similarities, cyclic-import, arguments-differ,
+          similarities, arguments-differ,
 
 
 
