Skip to content
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 #2826

Merged
merged 3 commits into from
Dec 2, 2024

Conversation

diegomarquezp
Copy link
Contributor

@diegomarquezp diegomarquezp commented Nov 21, 2024

This PR introduces java.time alternatives to existing org.threeten.bp.* methods, as well as switching internal variables (if any) to java.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).

@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: storage Issues related to the googleapis/java-storage API. labels Nov 21, 2024
@diegomarquezp diegomarquezp marked this pull request as ready for review November 21, 2024 22:51
@diegomarquezp diegomarquezp requested a review from a team as a code owner November 21, 2024 22:51
@diegomarquezp diegomarquezp requested a review from lqiu96 November 21, 2024 22:51
Comment on lines 675 to 676
public java.time.Duration getTerminationAwaitDurationDuration() {
return java.time.Duration.ofMinutes(1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we've seen this case before (or at least I don't recall). But if the method ends with Duration then we come across DurationDuration and I think that's even more confusing.

Maybe we want to keep this for consistency with all other SDK methods, but I'm thinking that naming scheme isn't ideal.

Perhaps getTerminationAwaitJavaTimeDuration()? Open to suggestions for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cross linking googleapis/java-datastore#1671 (comment)

I like JavaTimeDuration as the suffix. Let's use it in datastore too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the point in googleapis/java-datastore#1671 (comment) saying that using DurationJavaTime keeps the prefixes consistent

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to confirm this with @sydney-munro and @JesseLovelace

@diegomarquezp diegomarquezp requested a review from lqiu96 November 22, 2024 19:12
Copy link
Contributor

@lqiu96 lqiu96 left a 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

@sydney-munro sydney-munro merged commit baf30ee into main Dec 2, 2024
21 checks passed
@sydney-munro sydney-munro deleted the introduce-java-time branch December 2, 2024 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the googleapis/java-storage API. size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants