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

Removing Connection.lookup in storage. #588

Merged
merged 1 commit into from
Feb 6, 2015

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Feb 1, 2015

No description provided.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 1, 2015
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling bebc84d on dhermes:storage-remove-lookup into d858ff0 on GoogleCloudPlatform:master.

@dhermes dhermes added the api: storage Issues related to the Cloud Storage API. label Feb 1, 2015
return self.lookup(bucket_name) is not None
try:
self.get_bucket(bucket_name)
return True

This comment was marked as spam.

This comment was marked as spam.

@tseaver
Copy link
Contributor

tseaver commented Feb 3, 2015

Is there motivation for dropping lookup?

@dhermes
Copy link
Contributor Author

dhermes commented Feb 3, 2015

Motivation for dropping lookup is that

  1. It's essentially the same as get_bucket
  2. Connection methods should just be making API requests and leaving more user-friendly behavior to the other classes

@dhermes dhermes force-pushed the storage-remove-lookup branch from bebc84d to c506be0 Compare February 3, 2015 18:32
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling c506be0 on dhermes:storage-remove-lookup into 32b7853 on GoogleCloudPlatform:master.

@tseaver
Copy link
Contributor

tseaver commented Feb 3, 2015

I'm not sure I buy dropping lookup(): like dict.get(), it makes for smoother flow in calling code than always having to try: ... except: ... around KeyErrors/NotFound.

How likely is it that we will have applications dealing with multiple, non-default buckets? The more likely such cases, the less I'd be inclined to drop lookup().

@dhermes dhermes force-pushed the storage-remove-lookup branch from c506be0 to c4eb080 Compare February 5, 2015 00:41
@dhermes
Copy link
Contributor Author

dhermes commented Feb 5, 2015

@tseaver Rebased after #580.

I agree about smoother flow in calling code, but am working under the idea that Connection should not be the surface that users deal with (hence not in calling code).

The idea is to put the user-friendly type methods in a module like datastore.api and surface in the same way (like datastore.get, datastore.put and datastore.delete).

WDYT?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling c4eb080 on dhermes:storage-remove-lookup into 9518db7 on GoogleCloudPlatform:master.

@tseaver
Copy link
Contributor

tseaver commented Feb 5, 2015

So, do you see us adding API functions like storage.lookup():

def lookup(bucket_name, connection=None):
    """Look up a bucket by name.

    :type bucket_name: string
    :param bucket_name: the name of the bucket being looked up.

    :type connection: :class:`gcloud.storage.connection.Connection`, or None.
    :param connection: the connection to use.  If None, use the connection
                       inferred from the environment.

    :rtype: :class:`gcloud.storage.bucket.Bucket`, or None
    ;returns: the bucket, if it exists, or None if not.
    """

@dhermes
Copy link
Contributor Author

dhermes commented Feb 6, 2015

I do, with a fallback to the implicit connection. As with datastore, we should put it out of view for most users to reduce complexity / surface area.

@tseaver
Copy link
Contributor

tseaver commented Feb 6, 2015

Do you mean put the storage.lookup API out of sight?

@dhermes
Copy link
Contributor Author

dhermes commented Feb 6, 2015

Nope, I meant putting the Connection out of sight.

@tseaver
Copy link
Contributor

tseaver commented Feb 6, 2015

"Hiding" the Connection class SGTM.

@dhermes
Copy link
Contributor Author

dhermes commented Feb 6, 2015

Great. I'll follow this up with a creation of the storage.api module or you want me to do it here before an LGTM?

@tseaver
Copy link
Contributor

tseaver commented Feb 6, 2015

LGTM

dhermes added a commit that referenced this pull request Feb 6, 2015
Removing Connection.lookup in storage.
@dhermes dhermes merged commit 204389a into googleapis:master Feb 6, 2015
@dhermes dhermes deleted the storage-remove-lookup branch February 6, 2015 18:52
dhermes added a commit to dhermes/google-cloud-python that referenced this pull request Mar 12, 2015
This was originally removed in googleapis#588.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the Cloud Storage 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