Addressing review comments in PR #910.

In particular:
- Renaming `with_service_...` methods to `from_service_...` on
  `pubsub.Client`.
- Implementing `topic.subscription` factory.
- Using `topic.subscription` factory in docs and regression tests
  instead of instantiating a `Subscription`.
- Updating docstring for `http` on `pubsub.Client` to describe the
  default behavior when no value is passed.
- Making `client` a required / positional argument in `pubsub.Topic`
  (and ditching a test in the process).
- Renaming `test_make_topic` as `test_topic`.

Author: dhermes

docs/pubsub-usage.rst

@@ -12,9 +12,9 @@ Authorization / Configuration
 
 - The authentication credentials can be implicitly determined from the
   environment or directly via
-  :meth:`with_service_account_json <gcloud.pubsub.client.Client.with_service_account_json>`
+  :meth:`from_service_account_json <gcloud.pubsub.client.Client.from_service_account_json>`
   and
-  :meth:`with_service_account_p12 <gcloud.pubsub.client.Client.with_service_account_p12>`.
+  :meth:`from_service_account_p12 <gcloud.pubsub.client.Client.from_service_account_p12>`.
 
 - After setting ``GOOGLE_APPLICATION_CREDENTIALS`` and ``GCLOUD_PROJECT``
   environment variables, create a :class:`Client <gcloud.pubsub.client.Client>`
@@ -121,7 +121,7 @@ Create a new pull subscription for a topic:
    >>> from gcloud import pubsub
    >>> client = pubsub.Client()
    >>> topic = client.topic('topic_name')
-   >>> subscription = pubsub.Subscription('subscription_name', topic)
+   >>> subscription = topic.subscription('subscription_name')
    >>> subscription.create()  # API request
 
 Create a new pull subscription for a topic with a non-default ACK deadline:
@@ -131,7 +131,7 @@ Create a new pull subscription for a topic with a non-default ACK deadline:
    >>> from gcloud import pubsub
    >>> client = pubsub.Client()
    >>> topic = client.topic('topic_name')
-   >>> subscription = pubsub.Subscription('subscription_name', ack_deadline=90)
+   >>> subscription = topic.subscription('subscription_name', ack_deadline=90)
    >>> subscription.create()  # API request
 
 Create a new push subscription for a topic:
@@ -142,8 +142,8 @@ Create a new push subscription for a topic:
    >>> ENDPOINT = 'https://example.com/hook'
    >>> client = pubsub.Client()
    >>> topic = client.topic('topic_name')
-   >>> subscription = pubsub.Subscription('subscription_name',
-   ...                                    push_endpoint=ENDPOINT)
+   >>> subscription = topic.subscription('subscription_name',
+   ...                                   push_endpoint=ENDPOINT)
    >>> subscription.create()  # API request
 
 Check for the existence of a subscription:
@@ -153,7 +153,7 @@ Check for the existence of a subscription:
    >>> from gcloud import pubsub
    >>> client = pubsub.Client()
    >>> topic = client.topic('topic_name')
-   >>> subscription = pubsub.Subscription('subscription_name', topic)
+   >>> subscription = topic.subscription('subscription_name')
    >>> subscription.exists()  # API request
    True
 
@@ -165,7 +165,7 @@ Convert a pull subscription to push:
    >>> ENDPOINT = 'https://example.com/hook'
    >>> client = pubsub.Client()
    >>> topic = client.topic('topic_name')
-   >>> subscription = pubsub.Subscription('subscription_name', topic)
+   >>> subscription = topic.subscription('subscription_name')
    >>> subscription.modify_push_configuration(push_endpoint=ENDPOINT)  # API request
 
 Convert a push subscription to pull:
@@ -176,8 +176,8 @@ Convert a push subscription to pull:
    >>> ENDPOINT = 'https://example.com/hook'
    >>> client = pubsub.Client()
    >>> topic = client.topic('topic_name')
-   >>> subscription = pubusb.Subscription('subscription_name', topic,
-   ...                                    push_endpoint=ENDPOINT)
+   >>> subscription = topic.subscription('subscription_name',
+   ...                                   push_endpoint=ENDPOINT)
    >>> subscription.modify_push_configuration(push_endpoint=None)  # API request
 
 List subscriptions for a topic:
@@ -208,7 +208,7 @@ Delete a subscription:
    >>> from gcloud import pubsub
    >>> client = pubsub.Client()
    >>> topic = client.topic('topic_name')
-   >>> subscription = pubsub.Subscription('subscription_name', topic)
+   >>> subscription = topic.subscription('subscription_name')
    >>> subscription.delete()  # API request
 
 
@@ -222,7 +222,7 @@ Fetch pending messages for a pull subscription:
    >>> from gcloud import pubsub
    >>> client = pubsub.Client()
    >>> topic = client.topic('topic_name')
-   >>> subscription = pubsub.Subscription('subscription_name', topic)
+   >>> subscription = topic.subscription('subscription_name')
    >>> with topic.batch() as batch:
    ...     batch.publish('this is the first message_payload')
    ...     batch.publish('this is the second message_payload',
@@ -251,7 +251,7 @@ Fetch a limited number of pending messages for a pull subscription:
    >>> from gcloud import pubsub
    >>> client = pubsub.Client()
    >>> topic = client.topic('topic_name')
-   >>> subscription = pubsub.Subscription('subscription_name', topic)
+   >>> subscription = topic.subscription('subscription_name')
    >>> with topic.batch() as batch:
    ...     batch.publish('this is the first message_payload')
    ...     batch.publish('this is the second message_payload',
@@ -267,7 +267,7 @@ Fetch messages for a pull subscription without blocking (none pending):
    >>> from gcloud import pubsub
    >>> client = pubsub.Client()
    >>> topic = client.topic('topic_name')
-   >>> subscription = pubsub.Subscription('subscription_name', topic)
+   >>> subscription = topic.subscription('subscription_name')
    >>> received = subscription.pull(max_messages=1)  # API request
    >>> messages = [recv[1] for recv in received]
    >>> [message.id for message in messages]

gcloud/pubsub/client.py

@@ -40,7 +40,9 @@ class Client(object):
                         from the environment.
 
     :type http: :class:`httplib2.Http` or class that defines ``request()``.
-    :param http: An optional HTTP object to make requests.
+    :param http: An optional HTTP object to make requests. If not passed, an
+                 ``http`` object is created that is bound to the
+                 ``credentials`` for the current object.
 
     :raises: :class:`ValueError` if the project is neither passed in nor
              set in the environment.
@@ -58,7 +60,7 @@ def __init__(self, project=None, credentials=None, http=None):
         self.connection = Connection(credentials=credentials, http=http)
 
     @classmethod
-    def with_service_account_json(cls, json_credentials_path, project=None):
+    def from_service_account_json(cls, json_credentials_path, project=None):
         """Factory to retrieve JSON credentials while creating client.
 
         :type json_credentials_path: string
@@ -81,7 +83,7 @@ def with_service_account_json(cls, json_credentials_path, project=None):
         return cls(project=project, credentials=credentials)
 
     @classmethod
-    def with_service_account_p12(cls, client_email, private_key_path,
+    def from_service_account_p12(cls, client_email, private_key_path,
                                  project=None):
         """Factory to retrieve P12 credentials while creating client.
 

gcloud/pubsub/subscription.py

@@ -16,7 +16,6 @@
 
 from gcloud.exceptions import NotFound
 from gcloud.pubsub.message import Message
-from gcloud.pubsub.topic import Topic
 
 
 class Subscription(object):
@@ -63,6 +62,9 @@ def from_api_repr(cls, resource, client, topics=None):
         :rtype: :class:`gcloud.pubsub.subscription.Subscription`
         :returns: Subscription parsed from ``resource``.
         """
+        # pylint: disable=cyclic-import
+        from gcloud.pubsub.topic import Topic
+        # pylint: enable=cyclic-import
         if topics is None:
             topics = {}
         t_name = resource['topic']

gcloud/pubsub/test_client.py

@@ -82,7 +82,7 @@ def test_ctor_explicit(self):
         self.assertTrue(client_obj.connection._credentials is CREDS)
         self.assertEqual(CREDS._scopes, SCOPE)
 
-    def test_with_service_account_json(self):
+    def test_from_service_account_json(self):
         from gcloud._testing import _Monkey
         from gcloud.pubsub import client
         from gcloud.pubsub.connection import Connection
@@ -98,15 +98,15 @@ def mock_creds(arg1):
 
         BOGUS_ARG = object()
         with _Monkey(client, get_for_service_account_json=mock_creds):
-            client_obj = KLASS.with_service_account_json(
+            client_obj = KLASS.from_service_account_json(
                 BOGUS_ARG, project=PROJECT)
 
         self.assertEqual(client_obj.project, PROJECT)
         self.assertTrue(isinstance(client_obj.connection, Connection))
         self.assertTrue(client_obj.connection._credentials is CREDS)
         self.assertEqual(_CALLED, [(BOGUS_ARG,)])
 
-    def test_with_service_account_p12(self):
+    def test_from_service_account_p12(self):
         from gcloud._testing import _Monkey
         from gcloud.pubsub import client
         from gcloud.pubsub.connection import Connection
@@ -123,7 +123,7 @@ def mock_creds(arg1, arg2):
         BOGUS_ARG1 = object()
         BOGUS_ARG2 = object()
         with _Monkey(client, get_for_service_account_p12=mock_creds):
-            client_obj = KLASS.with_service_account_p12(
+            client_obj = KLASS.from_service_account_p12(
                 BOGUS_ARG1, BOGUS_ARG2, project=PROJECT)
 
         self.assertEqual(client_obj.project, PROJECT)
@@ -302,7 +302,7 @@ def test_list_subscriptions_with_topic_name(self):
                          % (PROJECT, TOPIC_NAME))
         self.assertEqual(req['query_params'], {})
 
-    def test_make_topic(self):
+    def test_topic(self):
         PROJECT = 'PROJECT'
         TOPIC_NAME = 'TOPIC_NAME'
         CREDS = _Credentials()

gcloud/pubsub/test_topic.py

@@ -24,9 +24,6 @@ def _getTargetClass(self):
     def _makeOne(self, *args, **kw):
         return self._getTargetClass()(*args, **kw)
 
-    def test_ctor_wo_client(self):
-        self.assertRaises(ValueError, self._makeOne, 'TOPIC_NAME', client=None)
-
     def test_ctor_w_explicit_timestamp(self):
         TOPIC_NAME = 'topic_name'
         PROJECT = 'PROJECT'
@@ -334,6 +331,20 @@ def test_delete_w_alternate_client(self):
         self.assertEqual(req['method'], 'DELETE')
         self.assertEqual(req['path'], '/%s' % PATH)
 
+    def test_subscription(self):
+        from gcloud.pubsub.subscription import Subscription
+        TOPIC_NAME = 'topic_name'
+        PROJECT = 'PROJECT'
+        CLIENT = _Client(project=PROJECT)
+        topic = self._makeOne(TOPIC_NAME,
+                              client=CLIENT)
+
+        SUBSCRIPTION_NAME = 'subscription_name'
+        subscription = topic.subscription(SUBSCRIPTION_NAME)
+        self.assertTrue(isinstance(subscription, Subscription))
+        self.assertEqual(subscription.name, SUBSCRIPTION_NAME)
+        self.assertTrue(subscription.topic is topic)
+
 
 class TestBatch(unittest2.TestCase):
 

gcloud/pubsub/topic.py

@@ -43,13 +43,32 @@ class Topic(object):
                                to the attributes of each published message:
                                the value will be an RFC 3339 timestamp.
     """
-    def __init__(self, name, client=None, timestamp_messages=False):
+    def __init__(self, name, client, timestamp_messages=False):
         self.name = name
-        if client is None:
-            raise ValueError('Topic constructor requires a client.')
         self._client = client
         self.timestamp_messages = timestamp_messages
 
+    def subscription(self, name, ack_deadline=None, push_endpoint=None):
+        """Creates a subscription bound to the current topic.
+
+        :type name: string
+        :param name: the name of the subscription
+
+        :type ack_deadline: int
+        :param ack_deadline: the deadline (in seconds) by which messages pulled
+                             from the back-end must be acknowledged.
+
+        :type push_endpoint: string
+        :param push_endpoint: URL to which messages will be pushed by the
+                              back-end. If not set, the application must pull
+                              messages.
+        """
+        # pylint: disable=cyclic-import
+        from gcloud.pubsub.subscription import Subscription
+        # pylint: enable=cyclic-import
+        return Subscription(name, self, ack_deadline=ack_deadline,
+                            push_endpoint=push_endpoint)
+
     @classmethod
     def from_api_repr(cls, resource, client):
         """Factory:  construct a topic given its API representation

regression/pubsub.py

@@ -18,7 +18,6 @@
 
 from gcloud import _helpers
 from gcloud import pubsub
-from gcloud.pubsub.subscription import Subscription
 
 
 _helpers._PROJECT_ENV_VAR_NAME = 'GCLOUD_TESTS_PROJECT_ID'
@@ -68,7 +67,7 @@ def test_create_subscription(self):
         topic.create()
         self.to_delete.append(topic)
         SUBSCRIPTION_NAME = 'subscribing-now'
-        subscription = Subscription(SUBSCRIPTION_NAME, topic)
+        subscription = topic.subscription(SUBSCRIPTION_NAME)
         self.assertFalse(subscription.exists())
         subscription.create()
         self.to_delete.append(subscription)
@@ -88,7 +87,7 @@ def test_list_subscriptions(self):
             'newest%d' % (1000 * time.time(),),
         ]
         for subscription_name in subscriptions_to_create:
-            subscription = Subscription(subscription_name, topic)
+            subscription = topic.subscription(subscription_name)
             subscription.create()
             self.to_delete.append(subscription)
 
@@ -106,7 +105,7 @@ def test_message_pull_mode_e2e(self):
         topic.create()
         self.to_delete.append(topic)
         SUBSCRIPTION_NAME = 'subscribing-now'
-        subscription = Subscription(SUBSCRIPTION_NAME, topic)
+        subscription = topic.subscription(SUBSCRIPTION_NAME)
         self.assertFalse(subscription.exists())
         subscription.create()
         self.to_delete.append(subscription)