Implement a GAX version of '_PublisherAPI'.

[tseaver]

Note: paging methods (list_topics and list_topic_subscriptions) are only partially implemented, because the GAX API does not permit passing in a page token (it cam be coerced into returning only a single page of results, and its token, though). See:

• Support sized pages in Page Streaming gax-python#86
• Change page-streaming behavior gax-python#94

[dhermes]

gcloud/pubsub/_gax.py:23:5: E303 too many blank lines (2)
gcloud/pubsub/_gax.py:30:5: E303 too many blank lines (2)
gcloud/pubsub/_gax.py:68:76: W291 trailing whitespace

[dhermes]

@tseaver To make the conditional definition comments a bit more clear let me describe what I have in mind (as a parallel to oauth2client.crypt).

1. You implement everything in gcloud.pubsub._gax at the top-level, allowing for import failures.
2. In the place where you use it (I assume gcloud.pubsub.connection, in a future PR?), there you conditionally import:

try:
    from gcloud.pubsub._gax import _PublisherAPI
    ...
    _HAVE_GAX = True
except ImportError:
    _PublisherAPI = None  # or maybe type(None) if you want it to be a class
    ...
    _HAVE_GAX = False

Having the conditional skip in test__gax rely on the import success also seems out of place, since we can just require the dependency in tox for Py2.7 and ignore it for other versions (I know the dependency is an extra right now in setup.py).

[tseaver]

You implement everything in gcloud.pubsub._gax at the top-level, allowing for import failures.

How do you deal with those errors being triggered by tests in the tox environments where the grpc extra cannot be installed? That is what unittest.skipUnless is for, after all.

[dhermes]

@tseaver You can still skipUnless, just use gcloud.pubsub.connection._HAVE_GAX.

[tseaver]

@dhermes

• The new module is the point where we're detecting the presence / absence of GAX.
• The future PR will add the import to gcloud.pubsub.client, not connection: it will test the _HAVE_GAX flag, along with an optional environment variable, to determine whether to use the _gax helpers or the JSON ones (from gcloud.pubsub.connection).
• Having tests of the "lower-level" module (gcloud.pubsub._gax) rely on the "higher-level" one (gcloud.pubsub.client) is just wrong.

[dhermes]

The way you detect it isn't particularly important to me. My feedback is not all-encompassing, just that you don't conditionally define an entire module.

[tseaver]

Don't conditionally define an entire module.

What is the difference for the user of the library? In this implementaiton, they can import gcloud.pubsub._gax without an exception, but the only member will be the _HAVE_GAX flag. In your version, trying to import gcloud.pubsub._gax would raise an ImportError.

WIthin gcloud, I can't see any win to deferring handling of the ImportError to the potential user (gcloud.pubsub.client). My way makes it easier to test the conditional code, because I can monkey-patch gcloud.pubsub._gax to have the desired value of _HAVE_GAX, as well as an appropriate stub for the helper objects.

[dhermes]

Anything named gcloud.pubsub._gax is not for users. I'm not asking for the change for users, but for code quality. It's harder to read and understand a class defined somewhere other than the top level.

You could even just have _gax.py contain:

try:
    import google.gax
except ImportError:
    _HAVE_GAX = False
else:
    _HAVE_GAX = True
    from gcloud.pubsub._gax_impl import _PublisherAPI

I'm just advocating against conditionally defined top-level objects for the same of the code.

[dhermes]

The cause stuff looks alright, so the last hangup is the conditionally defined stuff.

[tseaver]

@dhermes PTAL

[dhermes]

@tseaver Trying to review the "latest" changes but all the commits are from May 11 and May 12. Does a rebase change a commit date? Which ones are relevant since last review?

[tseaver]

@dhermes

@tseaver Trying to review the "latest" changes but all the commits are from May 11 and May 12. Does a rebase change a commit date?

I've never quite figured out what Github's UI was doing about dates.

Which [commits] are relevant since last review?

• 4877f5b
• a3cc3b1
• 46720fa

[dhermes]

LGTM. Sorry for the delay.