-
Notifications
You must be signed in to change notification settings - Fork 44
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
feat: introduce java.time
methods and variables
#1671
Conversation
...loud-datastore/src/main/java/com/google/cloud/datastore/admin/v1/DatastoreAdminSettings.java
Outdated
Show resolved
Hide resolved
/** Returns the total time to execute the query in the backend. */ | ||
public Duration getExecutionDuration() { | ||
public java.time.Duration getExecutionDurationDuration() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see DurationDuration
here as well. Hmm, let's see if we can a way to work around this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a couple ideas:
-
getExecutionDurationJavaTime
-
getExecutionDurationJT
-
getExecutionJavaTimeDuration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Responded in googleapis/java-storage#2826 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I think either getExecutionDurationJavaTime
or getExecutionJavaTimeDuration
should work. JT might be confusing to know that JT references java.time
.
I think getExecutionDurationJavaTime
actually sounds better since it keeps the original method name together getExecutionTime
. Let's get Cindy and the Storage SME to agree since I think we should be consistent on the choice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like getExecutionJavaTimeDuration
the best since it has the same suffix as other methods. But either getExecutionJavaTimeDuration
or getExecutionDurationJavaTime
sounds good to me. We can follow the same decision for Java-storage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once we settle JavaTimeDuration
or DurationJavaTime
This PR introduces
java.time
alternatives to existingorg.threeten.bp.*
methods, as well as switching internal variables (if any) tojava.time
The main constraint is to keep the changes backwards compatible, so for each existing threeten method "
method1(org.threeten.bp.Duration)
" we will add an alternative with a Duration (or Timestamp when applicable) suffix: "method1Duration(java.time.Duration)
".For most cases, the implementation will be held in the
java.time
method and the old threeten method will just delegate the call to it. However, for the case of abstract classes, the implementation will be kept in the threeten method to avoid breaking changes (i.e. users that already overloaded the method in their user code).