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

Jump to bottom

fix: Documentation for Blob.update() and Storage.update() methods is confusing/incorrect #261

Merged
merged 3 commits into from Apr 16, 2020
Merged

fix: Documentation for Blob.update() and Storage.update() methods is confusing/incorrect #261

merged 3 commits into from Apr 16, 2020

Conversation

dmitry-fa
Copy link
Contributor

Fixes #252

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 16, 2020
@dmitry-fa dmitry-fa added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 16, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 16, 2020
@dmitry-fa dmitry-fa changed the title fix: Documentation for Blob.update() and Storage.update() methods is confusing/incorrect. fix: Documentation for Blob.update() and Storage.update() methods is confusing/incorrect Apr 16, 2020
frankyn
frankyn requested changes Apr 16, 2020
View reviewed changes
Copy link
Member

@frankyn frankyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two nits, thanks @dmitry-fa!

google-cloud-storage/src/main/java/com/google/cloud/storage/Blob.java Outdated
* @param options update options
* @return a {@code Blob} object with updated information
* @param options preconditions to apply the update
* @return the updated blob
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the updated {@code Blob}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay

google-cloud-storage/src/main/java/com/google/cloud/storage/Blob.java
* the metadata generation of the current blob. If you want to update the information only if the
* current blob metadata are at their latest version use the {@code metagenerationMatch} option:
* {@code newBlob.update(BlobTargetOption.metagenerationMatch())}.
* Updates the blob properties. The {@code options} parameter contains the preconditions for
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd state {@code options} is BlobTargetOption and instead of preconditions use request options and link to update API documentation: https://cloud.google.com/storage/docs/json_api/v1/objects/update

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and link to update API documentation: https://cloud.google.com/storage/docs/json_api/v1/objects/update

done

I'd state {@code options} is BlobTargetOption

I think this is clear from the parameter type

and instead of preconditions use request options

Personally for me it was hard to understand the purspose of this parameter. So, I tried to rephrase this statement to help users not very familiar with the Storage details to get the meaning. I came up to: The options parameter contains the preconditions for applying the update

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, sgtm, I think having the link will help provide information.

@frankyn frankyn added the automerge Merge the pull request once unit tests and other checks pass. label Apr 16, 2020
@codecov
Copy link

codecov bot commented Apr 16, 2020 •
edited

Codecov Report

Merging #261 into master will decrease coverage by 0.06%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #261      +/-   ##
============================================
- Coverage     63.52%   63.46%   -0.07%     
  Complexity      540      540              
============================================
  Files            30       30              
  Lines          4776     4776              
  Branches        431      431              
============================================
- Hits           3034     3031       -3     
- Misses         1580     1583       +3     
  Partials        162      162
Impacted Files Coverage Δ Complexity Δ
...e/src/main/java/com/google/cloud/storage/Blob.java 81.97% <ø> (ø) 29.00 <0.00> (ø)
...rc/main/java/com/google/cloud/storage/Storage.java 80.57% <ø> (ø) 0.00 <0.00> (ø)
...gle/cloud/storage/testing/RemoteStorageHelper.java 61.47% <0.00%> (-2.46%) 9.00% <0.00%> (ø%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 41b6ad3...81285fa. Read the comment docs.

@gcf-merge-on-green gcf-merge-on-green bot merged commit 876405f into googleapis:master Apr 16, 2020
@dmitry-fa dmitry-fa deleted the UpdateBlob branch June 26, 2020 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frankyn frankyn frankyn approved these changes

@elharo elharo elharo approved these changes

Assignees
No one assigned
Labels
automerge Merge the pull request once unit tests and other checks pass. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Documentation for Blob.update() and Storage.update() methods is confusing/incorrect.
5 participants
@dmitry-fa @elharo @frankyn @googlebot @yoshi-kokoro