-
Notifications
You must be signed in to change notification settings - Fork 66
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
variables and methods
#1935
Conversation
// Returns a JSON String representation of the given duration. The JSON representation for a | ||
// Duration is a String that | ||
// ends in `s` to indicate seconds and is preceded by the number of seconds, with nanoseconds | ||
// expressed as fractional | ||
// seconds. | ||
String durationString(org.threeten.bp.Duration duration) { | ||
String durationStringDuration(java.time.Duration duration) { |
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.
This is package-private. I think we can straight replace
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.
Good catch. Overload removed
@Override | ||
public java.time.Duration getTotalRequestTimeoutDuration() { | ||
return firestoreOptions.getRetrySettings().getTotalTimeoutDuration(); |
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.
nit: Would be nice to get javadocs from firestore for 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.
@ehsannas for visibility - the superclass does not have javadocs on this method
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. Firestore team for final review
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).