feat: refactor model classes to prefer to java.time types #1394
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
77ce6cb to
39de58d
Compare
b09c998 to
a3e8e78
Compare
### What's been added Fields which map to apiary DateTime RFC 3339 fields will now also have a preferred java.time.OffsetDateTime getter & setter. Fields which map to apiary long to represent a duration in epoch millis will no also have a preferred java.time.Duration getter & setter. All new methods currently have the name of the corresponding java.time type appended to the property name. #### Refactor Long -> java.time.OffsetDateTime * BlobInfo#createTime * BlobInfo#customTime * BlobInfo#deleteTime * BlobInfo#retentionExpirationTime * BlobInfo#timeStorageClassUpdated * BlobInfo#updateTime * Blob#createTime * Blob#customTime * Blob#deleteTime * Blob#retentionExpirationTime * Blob#timeStorageClassUpdated * Blob#updateTime * BucketInfo.CreatedBeforeDeleteRule * BucketInfo.DeleteRule * BucketInfo.IsLiveDeleteRule * BucketInfo.NumNewerVersionsDeleteRule * BucketInfo.RawDeleteRule * BucketInfo#createTime * BucketInfo#updateTime * Bucket#createTime * Bucket#updateTime * Bucket#retentionEffectiveTime * HmacKey.HmacKeyMetadata#createTime * HmacKey.HmacKeyMetadata#updateTime #### Refactor Long -> java.time.Duration * BucketInfo#retentionPeriod * Bucket#retentionPeriod ### Deprecation of existing methods All existing "Long" based methods have been marked as `@Deprecated` and had their javadocs updated to point to the new preferred corresponding java.time method. When the existing "Long" based methods are removed, the corresponding java.time method will replace it at which point the java.time suffix methods will be deprecated for later cleanup. All existing "Long" based methods take care of performing the necessary conversion to the respective java.time type and will continue to work until their removal. ##### Why append the java.time type as a suffix? 1. Getters can't be overloaded, so we already need something to disambiguate which type should be returned. 2. For setters: we theoretically could overload since we have different method arguments, however, if a customer is already passing `null` as the method argument we've now created an ambiguity as to which method to invoke. By suffixing the setters as well, we avoid creating this possible ambiguity.
sydney-munro
approved these changes
May 24, 2022
Contributor
sydney-munro
left a comment
There was a problem hiding this comment.
LGTM just a small nit.
| @Deprecated | ||
| Builder setRetentionEffectiveTime(Long retentionEffectiveTime) { | ||
| return setRetentionEffectiveTime(millisOffsetDateTimeCodec.encode(retentionEffectiveTime)); | ||
| return setRetentionEffectiveTimeOffsetDateTime( |
Contributor
There was a problem hiding this comment.
nit: inconsistencies between style.
before you do
Builder setUpdateTime(Long updateTime) {
this.updateTime = millisOffsetDateTimeCodec.encode(updateTime);
return this;
}
vs calling the offset method in the deprecated old method.
Collaborator
Author
There was a problem hiding this comment.
Yeah, I agree this feels bad.
The method where the new one, calls the existing deprecated method is there to provide binary compatibility on the base builder. For reasons lost to time, the base builder is all public and someone theoretically could have extended it. In our implementations, I then do the proper order of the deprecated method calling the new method.
sydney-munro
pushed a commit
that referenced
this pull request
Jun 7, 2022
### What's been added Fields which map to apiary DateTime RFC 3339 fields will now also have a preferred java.time.OffsetDateTime getter & setter. Fields which map to apiary long to represent a duration in epoch millis will no also have a preferred java.time.Duration getter & setter. All new methods currently have the name of the corresponding java.time type appended to the property name. #### Refactor Long -> java.time.OffsetDateTime * BlobInfo#createTime * BlobInfo#customTime * BlobInfo#deleteTime * BlobInfo#retentionExpirationTime * BlobInfo#timeStorageClassUpdated * BlobInfo#updateTime * Blob#createTime * Blob#customTime * Blob#deleteTime * Blob#retentionExpirationTime * Blob#timeStorageClassUpdated * Blob#updateTime * BucketInfo.CreatedBeforeDeleteRule * BucketInfo.DeleteRule * BucketInfo.IsLiveDeleteRule * BucketInfo.NumNewerVersionsDeleteRule * BucketInfo.RawDeleteRule * BucketInfo#createTime * BucketInfo#updateTime * Bucket#createTime * Bucket#updateTime * Bucket#retentionEffectiveTime * HmacKey.HmacKeyMetadata#createTime * HmacKey.HmacKeyMetadata#updateTime #### Refactor Long -> java.time.Duration * BucketInfo#retentionPeriod * Bucket#retentionPeriod ### Deprecation of existing methods All existing "Long" based methods have been marked as `@Deprecated` and had their javadocs updated to point to the new preferred corresponding java.time method. When the existing "Long" based methods are removed, the corresponding java.time method will replace it at which point the java.time suffix methods will be deprecated for later cleanup. All existing "Long" based methods take care of performing the necessary conversion to the respective java.time type and will continue to work until their removal. ##### Why append the java.time type as a suffix? 1. Getters can't be overloaded, so we already need something to disambiguate which type should be returned. 2. For setters: we theoretically could overload since we have different method arguments, however, if a customer is already passing `null` as the method argument we've now created an ambiguity as to which method to invoke. By suffixing the setters as well, we avoid creating this possible ambiguity.
KengoTODA
added a commit
to KengoTODA/gradle-gcs-build-cache
that referenced
this pull request
Jan 13, 2023
refs googleapis/java-storage#1394 Signed-off-by: Kengo TODA <skypencil@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What's been added
Fields which map to apiary DateTime RFC 3339 fields will now also have
a preferred java.time.OffsetDateTime getter & setter.
Fields which map to apiary long to represent a duration in epoch millis
will no also have a preferred java.time.Duration getter & setter.
All new methods currently have the name of the corresponding java.time
type appended to the property name.
Refactor Long -> java.time.OffsetDateTime
Refactor Long -> java.time.Duration
Deprecation of existing methods
All existing "Long" based methods have been marked as
@Deprecatedand had their javadocs updated to point to the new preferred
corresponding java.time method.
When the existing "Long" based methods are removed, the corresponding
java.time method will replace it at which point the java.time suffix
methods will be deprecated for later cleanup.
All existing "Long" based methods take care of performing the necessary
conversion to the respective java.time type and will continue to work
until their removal.
Why append the java.time type as a suffix?
which type should be returned.
method arguments, however, if a customer is already passing
nullas themethod argument we've now created an ambiguity as to which method to
invoke. By suffixing the setters as well, we avoid creating this possible
ambiguity.
Part 2 of #1388