Skip to content

Conversation

@tseaver
Copy link
Contributor

@tseaver tseaver commented Mar 27, 2015

Uses #764 as a base.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 27, 2015
@tseaver tseaver mentioned this pull request Mar 27, 2015
9 tasks
@tseaver tseaver added the api: pubsub Issues related to the Pub/Sub API. label Mar 27, 2015
@dhermes
Copy link
Contributor

dhermes commented Mar 27, 2015

Got a lot of unused variables (I assume build failed). Also the do nothing setup and teardownModule aren't needed.

Other questions

  • Why the URI changes?
  • Do you plan on implementing more in this PR?

(Sorry for hacky review. On phone.)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling b906f33 on tseaver:pubsub-begin_regression_suite into b45a909 on GoogleCloudPlatform:master.

@tseaver
Copy link
Contributor Author

tseaver commented Mar 27, 2015

I rebased to remove #764 and adjust the tests to accommodate the changed URIs.

Per the API docs, the URIs are all based at https://pubsub.googleapis.com/v1beta2, rather than https://www.googleapis.com/pubsub/v1beta1. I know they are right because the regression tests were all 404ing with non-JSON errors before. :)

@tseaver
Copy link
Contributor Author

tseaver commented Mar 27, 2015

I plan to add more tests in later PRs (e.g., once we decide about returning Topic instances from pubsub.api.list_topics()).

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling c1ebe23 on tseaver:pubsub-begin_regression_suite into b45a909 on GoogleCloudPlatform:master.

@dhermes
Copy link
Contributor

dhermes commented Mar 27, 2015

Still have unused vars HTTP and SHARED_BUCKETS and an unused import httplib2.

Just call me the human linter :)

Other than LGTM (if you don't don't can you squash these fixed into the original commit, 4 commits for tint change is sadness)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 5981067 on tseaver:pubsub-begin_regression_suite into b45a909 on GoogleCloudPlatform:master.

@tseaver
Copy link
Contributor Author

tseaver commented Mar 27, 2015

@dhermes I removed those variables and the import, and squashed it down to two commits.

tseaver added a commit that referenced this pull request Mar 27, 2015
@tseaver tseaver merged commit 6d08741 into googleapis:master Mar 27, 2015
@dhermes
Copy link
Contributor

dhermes commented Mar 27, 2015

Looks great thanks

@tseaver tseaver deleted the pubsub-begin_regression_suite branch March 27, 2015 17:33
parthea pushed a commit that referenced this pull request Aug 21, 2025
parthea pushed a commit that referenced this pull request Sep 16, 2025
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