Skip to content

Conversation

@dhermes
Copy link
Contributor

@dhermes dhermes commented Oct 27, 2016

There is surprisingly no system test for Client.list_subscriptions(). After seeing how different the implementations of Client.list_subscriptions() and Topic.list_subscriptions() are, I am strongly for adding such a system test.

Sending this out without fixing unit tests to get it in the hands of reviewers. The unit tests may take me a non-trivial time, but I am working on them.

Also replacing _subscription_pb_to_mapping with MessageToDict.
Making it more specific to the methods that use it
(topic_list_subscriptions) since list_subscriptions will
also need to convert an item to a subscription.
Also remove GAX confusion in Pub/Sub between max_results and
page_size.
@dhermes dhermes added the api: pubsub Issues related to the Pub/Sub API. label Oct 27, 2016
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 27, 2016
These changes are needed to cover the change to iterators.
@dhermes
Copy link
Contributor Author

dhermes commented Oct 28, 2016

@tseaver @daspecster PTAL

Copy link
Contributor

@daspecster daspecster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM. @tseaver?

raise Conflict(topic_path)
raise
return _subscription_pb_to_mapping(sub_pb)
return MessageToDict(sub_pb)

This comment was marked as spam.

@dhermes dhermes merged commit 53c189a into googleapis:master Oct 29, 2016
@dhermes dhermes deleted the pubsub-iterators-3 branch October 29, 2016 16:06
richkadel pushed a commit to richkadel/google-cloud-python that referenced this pull request May 6, 2017
Converting Pub/Sub client->list_subscriptions to iterator.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: pubsub Issues related to the Pub/Sub API. cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants