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

<noun>() vs get_<noun>() vs <verb>_<noun>() #911

Closed
jgeewax opened this issue Jun 6, 2015 · 20 comments
Closed

<noun>() vs get_<noun>() vs <verb>_<noun>() #911

jgeewax opened this issue Jun 6, 2015 · 20 comments
Assignees
Labels
api: core type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@jgeewax
Copy link
Contributor

jgeewax commented Jun 6, 2015

Moved over from #910 per @dhermes

Goal of this issue is to decide:

  1. Is it reasonable to provide one-liners to do things like create_topic() which is just a wrapper:
    python def create_topic(self, name): topic = self.topic(name) topic.create() return topic
  2. Can we agree on the naming convention of what does and does not require an API request

Comment from the pull request was...

The name create_topic is ambiguous to new users. Does it mean create a Topic instance or actual insert one?

That's a good point. I think we should adopt the attitude that client.<verb>_<noun>() is an API request. So create_topic would hit the API trying to 'insert' the topic (using Google's API lingo). client.<noun> is nothing more than a factory for the noun which won't fire an API request but gives you back an instance of the .

We'd need to offer Client.topic and Client.create_topic side-by-side since users would need to construct pre-existing topics without sending insert.

Totally agree -- I think we had this debate already and I'm totally sold on the need for .topic() as the sole way to get a hold of a topic object, without hitting the API (as the topic literally has zero extra metadata).

For other APIs, where the object itself holds lots of (potentially important) metadata, I think it might be worthwhile to provide a get_<noun>() to pull down that metadata, in addition to the <noun>() method which just gives you an object which may not be tied to the remote (authoritative) representation.

An example here might be bucket.

  • .bucket() gives me a bucket object, sans remote data (no API request).
  • create_bucket() explicitly creates the bucket in the remote service, throwing an error if I was unable to do so.
  • get_bucket() would be an API request that gives me all the metadata about a bucket.

Note that the <verb>_bucket() methods are really just shortcuts so that I can have one-liners for the common things I want to do with a bucket. (ie, create == bucket = client.bucket('name'); bucket.create(); return bucket;)

In boto for S3, they have this verify or check parameter on the bucket constructor, so the comparisons look like:

Action boto gcloud (suggestion)
Get a bucket, no API request connection.get_bucket(validate=False) client.bucket()
Get a bucket + metadata connection.get_bucket() client.get_bucket()

I personally don't love the boto method, and would much rather the "gcloud (suggestion)" option.

@jgeewax jgeewax added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. api: core labels Jun 6, 2015
@dhermes
Copy link
Contributor

dhermes commented Jun 6, 2015

At a glance, I'll say this is mixing an imperative approach (functions only) with an object oriented approach. By making half the HTTP requests with functions and half with object methods, we muddle what our approach / philosophy is.

@jgeewax
Copy link
Contributor Author

jgeewax commented Jun 6, 2015

That is a good point. My concern is that our staunch adherence to the "only one approach" means more code for people in simple cases.

That is, if we stick to the object-oriented approach, one-liners aren't possible because of our decision to never allow chaining:

# This is *NOT* possible:
topic = client.topic('name').create()

If we go with the imperative approach, one liners are possible, but it means the logic gets mixed up a bit:

topic = client.create_topic()  # What does this return? What can you do with the result?

I'm suggesting we actively decide to make the object-oriented approach the authoritative one, with shortcut helper methods for common simple tasks where a common user would want a one-liner.

class Client(...):
  def create_topic(self, *args, **kwargs):
    topic = self.topic(*args, **kwargs)
    topic.create()
    return topic

This way, all logic lives in one place, but we get the one-liners a lot of users want (me among them)

@dhermes
Copy link
Contributor

dhermes commented Jun 6, 2015

I'm with most of it but "a lot of users want" isn't clearly true or false (you're the only user we really interact with).

@dhermes
Copy link
Contributor

dhermes commented Jun 6, 2015

I'm happy to implement, but would love to hear what @tseaver has to say.

@tseaver
Copy link
Contributor

tseaver commented Jun 9, 2015

I'm -1 on adding more topic- and subscription-specific methods to Client. Trading typing for clarity is not a win to me.

@jgeewax
Copy link
Contributor Author

jgeewax commented Jun 9, 2015

Where is the loss of clarity?

And how do I get a bucket and it's metadata? storage.bucket('...') doesn't do that does it?

@dhermes
Copy link
Contributor

dhermes commented Jun 9, 2015

from gcloud import storage
b = storage.get_bucket('name')

which is equivalent to

from gcloud import storage
b = storage.Bucket('name')
b.reload()

@jgeewax
Copy link
Contributor Author

jgeewax commented Jun 9, 2015

OK, so we already have the short-cut method for "getting" a bucket.. why not for creating?

from gcloud import storage
b = storage.create_bucket('name')

which would be equivalent to

from gcloud import storage
b = storage.Bucket('name')
b.create()

Right?

@dhermes
Copy link
Contributor

dhermes commented Jun 9, 2015

@jgeewax You ought to check out gcloud.storage.api, create_bucket also exists :)

FWIW The reason we have those bucket methods in storage.api is twofold

  • They were all already implemented on Connection previously
  • There is no "parent" for a bucket, so they are a top-level concept

@jgeewax
Copy link
Contributor Author

jgeewax commented Jun 9, 2015

So going back to the original thing... topic doesn't have a parent does it? So why wouldn't we add a shortcut method for create_topic too?

@dhermes
Copy link
Contributor

dhermes commented Jun 9, 2015

The only difference here is that there is no "all already implemented on Connection previously" baggage.

Before, we ported the methods blindly. Now, we can actually think about it.

FWIW, when we ported them @tseaver said he'd prefer we nuke storage.create_bucket and just recommend Bucket.create() and he has continued to mention it for awhile. I mention this to say that it's been clear on our minds, not just an ad hoc stance.

@jgeewax
Copy link
Contributor Author

jgeewax commented Jun 9, 2015

So by that measure we should nuke storage.get_bucket too -- and ask people to do .Bucket() and bucket.reload() -- right?

@dhermes
Copy link
Contributor

dhermes commented Jun 9, 2015

I don't have strong feelings here, I am just like the club secretary providing the minutes of previous meetings. @tseaver can weigh in

@jgeewax
Copy link
Contributor Author

jgeewax commented Jun 9, 2015

Sure -- my thoughts are that we should decide on something and be consistent. If it's just that the only verb we'll in-line is get, that's fine. I'd also like create and delete to be in-lined but my preference is in that order of priority. (get, then create, then delete). With pubsub, topics don't have any metadata, so get isn't really a big deal, however I think it still should exist...

@tseaver
Copy link
Contributor

tseaver commented Jun 10, 2015

I think keeping the responsibilities clear is important, and that we should be wary of adding "convenience" methods which confuse them. One symptom of those confused responsibilities is the injection of circular references (such as those @dhermes is fighting in #910 / #912). Another is an explosion of almost-but-not-quite-identical tests for the More Than One Way To Do It implementations.

@jgeewax
Copy link
Contributor Author

jgeewax commented Jun 10, 2015

Re circular references:

Totally get that -- So if that's the case, shouldn't we chop out get_bucket and similar?

Re test explosion:

Should we only have one test for these? Something superficial that says "this should pass along all arguments and call this method... "? And then all the actual test logic is on the real method?

@dhermes
Copy link
Contributor

dhermes commented Aug 31, 2015

We currently have Client.fetch_project and Client.new_project in Resource Manager, then Client.get_bucket (makes API request) and Client.bucket (factory) in Storage. We don't really have this conflict elsewhere, but should really get our story straight.

Also, these are really the only places that a factory is side-by-side with an API method that does Noun.reload().

@jgeewax
Copy link
Contributor Author

jgeewax commented Sep 1, 2015

fetch_noun() (AFAIK) should just be noun = client.noun() and noun.reload() in one go....

@dhermes
Copy link
Contributor

dhermes commented Sep 1, 2015

My comment was about how to implement, but about what to implement / what to name the things that are implemented.

We are self-inconsistent right now in our offerings and I'd like to become self-consistent.

@dhermes
Copy link
Contributor

dhermes commented Dec 31, 2015

This conversation seems to have lost steam on both sides. Closing out, but people can re-open if they think something should be done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: core type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

No branches or pull requests

3 participants