feat: add support of customTime metadata#413
feat: add support of customTime metadata#413JesseLovelace merged 7 commits intogoogleapis:masterfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## master #413 +/- ##
============================================
+ Coverage 63.19% 64.58% +1.38%
- Complexity 613 617 +4
============================================
Files 32 32
Lines 5144 5232 +88
Branches 492 507 +15
============================================
+ Hits 3251 3379 +128
+ Misses 1729 1694 -35
+ Partials 164 159 -5
Continue to review full report at Codecov.
|
70aa315 to
6e36ce5
Compare
6e36ce5 to
50df38f
Compare
frankyn
left a comment
There was a problem hiding this comment.
I have a few nits for documentation, otherwise LGTM.
google-cloud-storage/src/main/java/com/google/cloud/storage/BlobInfo.java
Outdated
Show resolved
Hide resolved
| public abstract Builder setCrc32c(String crc32c); | ||
|
|
||
| /** Sets the custom time for an object. */ | ||
| public abstract Builder setCustomTime(Long customTime); |
There was a problem hiding this comment.
Why is this abstract? If it's concrete it wouldn't be a breaking change.
There was a problem hiding this comment.
It's the development model we've used over the years. It was designed before my time and folks who worked on the library have since left the team.
We could update it to concrete method instead but we would start changing development flow. Maybe that's a good thing, but I'd prefer to prevent changing development flow until we have a new design that fits into that model.
There was a problem hiding this comment.
The pot roast principle applies:
https://www.psychologytoday.com/us/blog/thinking-makes-it-so/201402/the-pot-roast-principle
There was a problem hiding this comment.
If it's implemented, it could be implemented to raise an exception of it not being implemented. Seems a little clunky, but would remove the need to create a breaking change.
Thanks for raising the simple questions @elharo, I haven't applied this train of thought to this problem, and I appreciate the feedback.
There was a problem hiding this comment.
@athakor please implement this method instead of using abstract to prevent the breaking change. Instead raise an exception of unimplemented if called directly.
|
@frankyn PTAL |
|
|
||
| /** | ||
| * Sets the custom time for an object. Once set it can't be unset and only changed to a custom | ||
| * datetime in the future. If the custom_time must be unset, you must either perform a rewrite |
There was a problem hiding this comment.
Don't use "custom_time" in this context, say "custom time" instead. Replace "If the custom_time must be unset," with "To unset the custom time,"
| * datetime in the future. If the custom_time must be unset, you must either perform a rewrite | ||
| * operation or upload the data again. | ||
| * | ||
| * <p>Example of setting the custom_time. |
There was a problem hiding this comment.
custom time instead of custom_time
|
@frankyn @JesseLovelace All the comments have been addressed PTAL |
elharo, PR addressed the remaining comment about the breaking change while also raising an exception of unimplemented if used directly. Dismissing to merge.
…rage into addolmexception
Towards #395