Introduce Refaster rules to streamline `java.time` type creation #322

CoolTomatos · 2022-10-28T12:07:41Z

No description provided.

Stephan202

Thanks for the PR, @CoolTomatos! I can you share some further details on why he "fluent" expressions should be preferred? (If helpful, feel free to reach out on Slack to link to an associated internal discussion.)

Stephan202 · 2022-10-29T11:14:31Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/TimeRules.java

+
+    @AfterTemplate
+    LocalDate after(Instant instant, ZoneOffset zoneOffset) {
+      return instant.atOffset(zoneOffset).toLocalDate();


While this is more "OOP-y" than the original, I'm not yet convinced that these rewrites are an improvement:

The old code better conveys intent (IMHO).

The old code seems more efficient.

So I wonder: shouldn't we define these rules in the opposite direction?

Hi @Stephan202,
The four templates I added here is to extend the existing InstantAtOffset (which I renamed).
I don't have strong arguments about one way or another.

Cc @rickie

I have no strong preference here to be honest. So let's do it the other way around then :).

Doing it the other way around SGTM! (I think that the existing OffsetDateTime.ofInstant(instant, zoneOffset) -> instant.atOffset(zoneOffset) makes sense, but for the new cases the static factory method seems to be the way to go 👍.)

Stephan202

Added a commit. I suspect we may need to tweak the @BeforeTemplates a bit, but need to leave the metro now. More later (after a bunch of meetings).

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/TimeRules.java

Stephan202

Rebased and added a commit. Nice set of rules like this!

Suggested commit message:

Introduce Refaster rules for canonical `java.time` type creation (#322)

(Not yet really happy with this suggestion; ideas welcome.)

Stephan202 · 2022-11-03T18:01:45Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/TimeRules.java

+    LocalDate before(Instant instant, ZoneId zoneId) {
+      return Refaster.anyOf(
+          instant.atZone(zoneId).toLocalDate(),
+          instant.atZone(zoneId).toOffsetDateTime().toLocalDate(),


We can have a separate instant.atZone(zoneId).toOffsetDateTime() -> OffsetDateTime.ofInstant(instant, zoneId) rule :)

Stephan202 · 2022-11-03T18:36:26Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/TimeRules.java

+  static final class LocalTimeOfInstant {
+    @BeforeTemplate
+    LocalTime before(Instant instant, ZoneId zoneId) {
+      return Refaster.anyOf(
+          instant.atZone(zoneId).toLocalTime(),
+          instant.atZone(zoneId).toOffsetDateTime().toLocalTime(),
+          LocalDateTime.ofInstant(instant, zoneId).toLocalTime());
+    }


One could also go via OffsetTime; will add.

rickie

One question, other than that, LGTM!

Nice PR @CoolTomatos 🚀 !

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/TimeRules.java

rickie · 2022-11-03T20:39:29Z

@chamil-prabodha anything from your side :)?

chamil-prabodha

Looks good to me. I added one small comment in the tests.
Nice PR! This would definitely help us a lot to clean up the code 😄

...rone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/TimeRulesTestInput.java

Stephan202

And rebased. :)

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/TimeRules.java

… more fluent way.

rickie · 2022-11-04T13:21:09Z

I can only come up with: to streamline 'java.time' type creation.
Without looking at InstantAtZone, as it is an exception to the rule (or suggested commit message for that matter), it would make sense to call it something like ... prefer '#ofInstant' method for creating 'java.time' types.

(I'm using ' in the examples to not break the )

Anyway, if you don't like the suggestion, I'd suggest to roll with the proposed one.

@CoolTomatos thanks for the extra rebase 😉.

CoolTomatos · 2022-11-04T13:54:54Z

I don't have merge rights so please go ahead with your suggestion captain @rickie 😁

Stephan202 · 2022-11-04T18:19:09Z

So either:

Introduce Refaster rules for canonical `java.time` type creation (#322)
Introduce Refaster rules to streamline `java.time` type creation (#322)

I guess I'm still tending to the first option. Hard to put my finger on it, but it seems to make a more modest claim 🤔. Regardless: @rickie will let you merge :)

rickie · 2022-11-04T18:26:37Z

@rickie will let you merge :)

Hehe it that case I'm going with the second one 👀:smile:.

CoolTomatos requested review from Stephan202, chamil-prabodha and rickie October 28, 2022 12:07

CoolTomatos self-assigned this Oct 28, 2022

Stephan202 reviewed Oct 29, 2022

View reviewed changes

rickie added the new feature label Oct 31, 2022

rickie added this to the 0.6.0 milestone Oct 31, 2022

CoolTomatos force-pushed the CoolTomatos/instant_related_templates branch 5 times, most recently from 4478615 to 413ef61 Compare October 31, 2022 22:31

CoolTomatos requested a review from Stephan202 October 31, 2022 22:38

Stephan202 reviewed Nov 1, 2022

View reviewed changes

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/TimeRules.java Outdated Show resolved Hide resolved

CoolTomatos force-pushed the CoolTomatos/instant_related_templates branch from bbd1efb to b8af367 Compare November 1, 2022 19:04

CoolTomatos requested a review from Stephan202 November 2, 2022 15:08

Stephan202 force-pushed the CoolTomatos/instant_related_templates branch from b8af367 to 36b5c80 Compare November 3, 2022 18:39

Stephan202 approved these changes Nov 3, 2022

View reviewed changes

rickie approved these changes Nov 3, 2022

View reviewed changes

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/TimeRules.java Outdated Show resolved Hide resolved

chamil-prabodha approved these changes Nov 3, 2022

View reviewed changes

...rone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/TimeRulesTestInput.java Outdated Show resolved Hide resolved

Stephan202 force-pushed the CoolTomatos/instant_related_templates branch from 36b5c80 to 3712722 Compare November 4, 2022 04:33

Stephan202 approved these changes Nov 4, 2022

View reviewed changes

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/TimeRules.java Outdated Show resolved Hide resolved

CoolTomatos and others added 5 commits November 4, 2022 12:36

Added template to convert Instants to different Temporal classes in a…

3cebc75

… more fluent way.

Applied suggestion from @Stephan202

678d47f

Suggestions

8a08e8c

Suggestions

7b091a8

Swap things

5191427

CoolTomatos force-pushed the CoolTomatos/instant_related_templates branch from 3712722 to 5191427 Compare November 4, 2022 11:36

rickie changed the title ~~Added templates to convert Instants to different Temporal classes in a more fluent way.~~ Introduce Refaster rules to streamline java.time type creation Nov 4, 2022

rickie merged commit 281534a into master Nov 4, 2022

rickie deleted the CoolTomatos/instant_related_templates branch November 4, 2022 18:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Refaster rules to streamline `java.time` type creation #322

Introduce Refaster rules to streamline `java.time` type creation #322

CoolTomatos commented Oct 28, 2022

Stephan202 left a comment

Stephan202 Oct 29, 2022

CoolTomatos Oct 31, 2022

rickie Oct 31, 2022

Stephan202 Oct 31, 2022

CoolTomatos Oct 31, 2022

Stephan202 left a comment

Stephan202 left a comment

Stephan202 Nov 3, 2022

Stephan202 Nov 3, 2022

rickie left a comment

rickie commented Nov 3, 2022

chamil-prabodha left a comment

Stephan202 left a comment

rickie commented Nov 4, 2022

CoolTomatos commented Nov 4, 2022 •

edited

Loading

Stephan202 commented Nov 4, 2022

rickie commented Nov 4, 2022

Introduce Refaster rules to streamline java.time type creation #322

Introduce Refaster rules to streamline java.time type creation #322

Conversation

CoolTomatos commented Oct 28, 2022

Stephan202 left a comment

Choose a reason for hiding this comment

Stephan202 Oct 29, 2022

Choose a reason for hiding this comment

CoolTomatos Oct 31, 2022

Choose a reason for hiding this comment

rickie Oct 31, 2022

Choose a reason for hiding this comment

Stephan202 Oct 31, 2022

Choose a reason for hiding this comment

CoolTomatos Oct 31, 2022

Choose a reason for hiding this comment

Stephan202 left a comment

Choose a reason for hiding this comment

Stephan202 left a comment

Choose a reason for hiding this comment

Stephan202 Nov 3, 2022

Choose a reason for hiding this comment

Stephan202 Nov 3, 2022

Choose a reason for hiding this comment

rickie left a comment

Choose a reason for hiding this comment

rickie commented Nov 3, 2022

chamil-prabodha left a comment

Choose a reason for hiding this comment

Stephan202 left a comment

Choose a reason for hiding this comment

rickie commented Nov 4, 2022

CoolTomatos commented Nov 4, 2022 • edited Loading

Stephan202 commented Nov 4, 2022

rickie commented Nov 4, 2022

Introduce Refaster rules to streamline `java.time` type creation #322

Introduce Refaster rules to streamline `java.time` type creation #322

CoolTomatos commented Nov 4, 2022 •

edited

Loading