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

Allow setting 'bucket.storage_class'. #1213

Merged
merged 1 commit into from
Nov 10, 2015
Merged

Allow setting 'bucket.storage_class'. #1213

merged 1 commit into from
Nov 10, 2015

Conversation

tseaver
Copy link
Contributor

@tseaver tseaver commented Nov 6, 2015

Fixes #1030.

@tseaver tseaver added the api: storage Issues related to the Cloud Storage API. label Nov 6, 2015
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 6, 2015
@@ -569,6 +569,7 @@ def test_location_setter(self):
self.assertEqual(bucket.location, None)
bucket.location = 'AS'
self.assertEqual(bucket.location, 'AS')
self.assertTrue('location' in bucket._changes)

This comment was marked as spam.

@dhermes
Copy link
Contributor

dhermes commented Nov 8, 2015

Only one question, why not add to the constructor? Or make the constructor take **kwargs** that we can farm out to setters.

@tseaver
Copy link
Contributor Author

tseaver commented Nov 9, 2015

why not add to the constructor?

We have four settable properties: cors, lifecycle_rules, storage_class, and versioning_enabled: we would likely need to allow any of them to be set, if we wanted this feature.

Or make the constructor take kwargs that we can farm out to setters.

Ugh: we lose Python's type checking, and have to add it in ourselves.

@dhermes
Copy link
Contributor

dhermes commented Nov 9, 2015

How you mean type checking? Do the setters do any? Wouldn't they still?

@tseaver
Copy link
Contributor Author

tseaver commented Nov 9, 2015

How you mean type checking? Do the setters do any? Wouldn't they still?

That was an objection to using **kw, vs. explicitly-named arguments. All of those properties are rare enough cases that we can leave them out of the constructor.

@dhermes
Copy link
Contributor

dhermes commented Nov 10, 2015

Gotcha. So WDYT about just adding storage_class to the constructor (or all four)?

@tseaver
Copy link
Contributor Author

tseaver commented Nov 10, 2015

WDYT about just adding storage_class to the constructor (or all four)?

👎 Seems like a complication for a pretty rare case, with an easy-to-understand alternative already present.

@dhermes
Copy link
Contributor

dhermes commented Nov 10, 2015

OK LGTM

(Responding from email for first time ever. Like whoa. Weird.)

Danny Hermes
On Nov 10, 2015 6:21 PM, "Tres Seaver" [email protected] wrote:

WDYT about just adding storage_class to the constructor (or all four)?

[image: 👎] Seems like a complication for a pretty rare case, with an
easy-to-understand alternative already present.


Reply to this email directly or view it on GitHub
#1213 (comment)
.

tseaver added a commit that referenced this pull request Nov 10, 2015
…A_buckets

Allow setting 'bucket.storage_class'.
@tseaver tseaver merged commit b71ac51 into googleapis:master Nov 10, 2015
@tseaver tseaver deleted the 1030-allow_creating_nearline_DRA_buckets branch November 10, 2015 18:53
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.

3 participants