Conversation

dmitry-fa

Fixes #252

@googlebotgooglebot added the cla: yesThis human has signed the Contributor License Agreement.label Apr 16, 2020
@dmitry-fadmitry-fa requested review from frankyn and elharo April 16, 2020 12:21
elharo
elharo approved these changes Apr 16, 2020
View reviewed changes
@dmitry-fadmitry-fa added the kokoro:force-runAdd this label to force Kokoro to re-run the tests.label Apr 16, 2020
@yoshi-kokoroyoshi-kokoro removed the kokoro:force-runAdd this label to force Kokoro to re-run the tests.label Apr 16, 2020
@dmitry-fadmitry-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 suggested changes Apr 16, 2020
View reviewed changes

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
Contributor

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
Contributor

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
Contributor

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.

@frankynfrankyn added the automergeMerge the pull request once unit tests and other checks pass.label Apr 16, 2020
@codecov

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 FilesCoverage ΔComplexity Δ
...e/src/main/java/com/google/cloud/storage/Blob.java81.97% <ø> (ø)29.00 <0.00> (ø)
...rc/main/java/com/google/cloud/storage/Storage.java80.57% <ø> (ø)0.00 <0.00> (ø)
...gle/cloud/storage/testing/RemoteStorageHelper.java61.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-greengcf-merge-on-green bot merged commit 876405f into googleapis:master Apr 16, 2020
@dmitry-fadmitry-fa deleted the UpdateBlob branch June 26, 2020 07:34
Sign up for free to join this conversation on . Already have an account? Sign in to comment

@frankyn frankyn frankyn approved these changes

@elharo elharo elharo approved these changes

No one assigned
automergeMerge the pull request once unit tests and other checks pass.cla: yesThis human has signed the Contributor License Agreement.
None yet
No milestone

Successfully merging this pull request may close these issues.

Documentation for Blob.update() and Storage.update() methods is confusing/incorrect.