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

Separating _patch_properties and PATCH on Bucket. #683

Merged
merged 1 commit into from
Mar 5, 2015

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Feb 26, 2015

@tseaver there is plenty to discuss here and I will enumerate.

In the immediately relevant category is my uses of self.patch() in several of the methods. I was unclear if we think they should be setters or actual API requests.

Relates to #632.

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

dhermes commented Feb 26, 2015

Some other things to worry about

  1. In the future where connection is not bound to Bucket
    def patch(self, connection=None):
        self._properties = connection.api_request(...

This would be a 204 on a Batch and do very much the wrong thing
2. Most getters use _PropertyMixin.properties, which is always an API request (See #688)
3. Blob.metadata (setter) is the only use of _patch_properties there in Blob. It seems _scalar_property was the biggest user. (See #690, it changes _patch_properties everywhere.)
4. This may break _PropertyBatch as it replaces _patch_properties on enter and puts it back on exit. (See #690, it changes __exit__.)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling cdd6694 on dhermes:patch-bucket-imperative into 51e286f on GoogleCloudPlatform:master.

@dhermes
Copy link
Contributor Author

dhermes commented Mar 3, 2015

@tseaver PTAL

@tseaver
Copy link
Contributor

tseaver commented Mar 5, 2015

LGTM.

dhermes added a commit that referenced this pull request Mar 5, 2015
Separating _patch_properties and PATCH on Bucket.
@dhermes dhermes merged commit ef1a3da into googleapis:master Mar 5, 2015
@dhermes dhermes deleted the patch-bucket-imperative branch March 5, 2015 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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