Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding auto create to pub/sub topic. #949

Closed
wants to merge 1 commit into from

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Jun 25, 2015

Fixes #905.


This is labelled don't merge because it doesn't adhere to the truly optimal case described in #905 by @jonparrott. I sent the PR just to have more discussion here (now that @tseaver is back in the US).

I'm worried that implementing the method used by node will be painful. I suppose we could do it with something like a custom class that is just bound to the topic and calls made for it

class CustomHttp(object):

    def __init__(self, http, object_to_create):
        self._http = http
        self._object_to_create = object_to_create
        self._creation_checked = False

    def request(self, method, uri=None, headers=None, body=None):
        # Just do the request
        try:
            self._http(method, uri=uri, headers=headers, body=body)
        except:
            # Do some other stuff

BUT, no matter how I slice it, the added complexity doesn't seem to be worth it for a measly feature like auto_create. But maybe I am undervaluing it?

@dhermes dhermes added do not merge Indicates a pull request not ready for merge, due to either quality or timing. api: pubsub Issues related to the Pub/Sub API. labels Jun 25, 2015
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 25, 2015
@dhermes dhermes added do not merge Indicates a pull request not ready for merge, due to either quality or timing. and removed do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels Jun 25, 2015
@dhermes
Copy link
Contributor Author

dhermes commented Jun 25, 2015

It looks like I also needed to delete the reviewninja webhook (see #948 for original reviewninja post) from the GitHub settings. Lets hope it stops bothering us 😄

@theacodes
Copy link
Contributor

To be honest, in python looking before you leap isn't as overly onerous as it is in node:

if not topic.exists():
    topic.create()

So I'm okay with nixing this if the cost/benefit isn't good. My only issue now is that the check for exists is slow, but that's not this library's fault.

@dhermes
Copy link
Contributor Author

dhermes commented Jun 30, 2015

@jonparrott Sorry for the huge delay in reply.

Since it was @waprin who filed, I want to feel out for how he feels about closing #905.

Also I noticed on googleapis/google-cloud-node#696 that you and @tmatsuo are having some reservations of autoCreate and/or think that exists() is a fine substitute for autoCreate.

@waprin
Copy link
Contributor

waprin commented Jun 30, 2015

Yeah in retrospect auto_create creates more confusion than convenience, I'm fine with just closing them all down.

@dhermes dhermes closed this Jun 30, 2015
@dhermes
Copy link
Contributor Author

dhermes commented Jun 30, 2015

Thanks guys! Let me know if there is anything we can do to make the sample writing process easier.

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. do not merge Indicates a pull request not ready for merge, due to either quality or timing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants