`TimeTemplates`: Introduce `Duration.ofX` templates #41

Badbond · 2022-02-18T17:14:41Z

Introduces templates for Duration.of(amount, ChronoUnit.X) to Duration.ofX(amount) applicable to nanos, millis, seconds, minutes, hours and days.

Duration.ofSeconds(amount) does have an overload to specify nanoAdjustment but AFAICS defaults to 0 in both before & after.

Added a single test to see if it also works with static imports (but if obviously redundant, we can remove it).

Fixed some grammar in underlying JavaDoc.

...ne-contrib/src/test/resources/tech/picnic/errorprone/bugpatterns/TimeTemplatesTestInput.java

rickie

PR looks good! 🚀

Left some small tips 😄.

rickie · 2022-02-22T13:01:14Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/TimeTemplates.java

@@ -310,8 +311,80 @@ Duration after() {
    }
  }

+  static final class DurationOfNanos {


Almost every template in this class has a Javadoc, let's add it to these templates as well :).

Perhaps something along the lines of (just a suggestion):
Prefer ... over more contrived alternatives.

The templates are now based on the metric precision, which makes perfect sense. When looking at the implementation(s) of the Duration.of{...}(amount); methods, they are ordered lexicographically.
Based on that + knowing that @Stephan202 is a fan of lexicographical ordering, we could change the sorting to be lexicographical as well 😄.

Yeah couldn't get a nice description for docs. Also not sure if I'd agree with the term 'contrived'. Based on the LocalTimeMin template I went with more basic:
/** Prefer {@link Duration#ofNanos(long)} over alternative representations. */ (and similar). Open to suggestions.

Regarding ordering, the implementation is not entirely lexicographically ordered 😄 ofSeconds is not in the correct place and ofMillis should be before isMinutes. Also TimeTemplateTestInput#testZeroDuration() orders in time magnitude.

But I don't have a strong preference for order (go Rebel Alliance!) so ordered them lexicographical now instead of in order of time magnitude (insert joke about Yoda time here) as you prefer.

For contrived I think this definition fits the context better: created or arranged in a way that seems artificial and unrealistic..
Nevertheless, good suggestion, SGTM!

Other suggestion would be "more verbose examples/representation" as we now don't need ChronoUnit 😄.

Regarding ordering, the implementation is not entirely lexicographically ordered

Did only check the first four methods, which was not enough apparently 😄.

rickie · 2022-02-22T13:07:47Z

...ne-contrib/src/test/resources/tech/picnic/errorprone/bugpatterns/TimeTemplatesTestInput.java

+    return Duration.of(1, ChronoUnit.DAYS);
+  }
+
+  Duration testDurationOfDaysStaticImport() {


As you already pointed out in the PR description, indeed we do not need the static import case. Refaster is smart enough to also match this, so we don't need to have a test for this 😄.

Useful to know: if we want to have multiple test cases for one Refaster template, we generally wrap these cases in an ImmutableSet.of(...) and return that. This way we have the test cases together in one single method.

Within this class there are other methods that do this. See for example:
ImmutableSet<Duration> testZeroDuration() {

As a result the tests are more concise and it is easier to see how a Refaster template is tested.

Regarding the static import test, this is now removed as suggested. Therefore, also no grouping required.

...ne-contrib/src/test/resources/tech/picnic/errorprone/bugpatterns/TimeTemplatesTestInput.java

This reverts commit bf8458f.

rickie · 2022-02-23T09:18:49Z

Nice 🚀 !

mussatto

LGTM :)

rickie · 2022-02-24T10:41:46Z

Suggested commit message:

TimeTemplates: Rewrite `Duration.of(n, ChronoUnit.X)` to `Duration.ofX(n)`

Add templates for: `Duration.of{Nanos,Millis,Seconds,Minutes,Hours,Days}(n)`.

Badbond · 2022-02-24T13:09:19Z

Updated the commit message to have X recurring in both LHS and RHS.

werli

Nice to see these getting added. 💪

werli · 2022-02-24T13:15:07Z

...e-contrib/src/test/resources/tech/picnic/errorprone/bugpatterns/TimeTemplatesTestOutput.java

+  Duration testDurationOfDays() {
+    return Duration.ofDays(1);
+  }
+
+  Duration testDurationOfHours() {
+    return Duration.ofHours(1);
+  }
+
+  Duration testDurationOfMillis() {
+    return Duration.ofMillis(1);
+  }
+
+  Duration testDurationOfMinutes() {
+    return Duration.ofMinutes(1);
+  }
+
+  Duration testDurationOfNanos() {
+    return Duration.ofNanos(1);
+  }
+
+  Duration testDurationOfSeconds() {
+    return Duration.ofSeconds(1);
+  }
+


Interestingly, I'd have expected this to be sorted by time units (days, hours, minutes, seconds, millis, nanos), but this works for me as well.

See discussion here. I'm open to both 😄

but this works for me as well.

Will keep true to my words here. 🙂

Stephan202

Verified that the build also passes against master. Will merge with a slightly tweaked suggested commit message:

Prefer `Duration.ofX(n)` over `Duration.of(n, ChronoUnit.X)` (#41)

`TimeTemplates` now contains Refaster templates for 
`Duration.of{Nanos,Millis,Seconds,Minutes,Hours,Days}(n)`.

Badbond added 2 commits February 18, 2022 17:42

Add Duration.ofMillis template

af9fb6b

Add remaining Duration.ofX methods

4420491

Badbond requested a review from rickie February 18, 2022 17:14

Badbond force-pushed the pdsoels/time-templates-duration-of branch from 4ecd48a to d813d5b Compare February 18, 2022 17:22

Add static import test

0b62344

Badbond force-pushed the pdsoels/time-templates-duration-of branch from d813d5b to 0b62344 Compare February 18, 2022 17:44

Badbond commented Feb 18, 2022

View reviewed changes

...ne-contrib/src/test/resources/tech/picnic/errorprone/bugpatterns/TimeTemplatesTestInput.java Outdated Show resolved Hide resolved

Badbond requested a review from mussatto February 21, 2022 08:21

Add ChronoUnit.DAYS static import to exemptions

27813a8

rickie requested changes Feb 22, 2022

View reviewed changes

Badbond added 2 commits February 22, 2022 16:05

Add JavaDoc and order lexicographically

9330174

Group test input and output

bf8458f

Badbond requested a review from rickie February 22, 2022 15:13

rickie mentioned this pull request Feb 22, 2022

Introduce ImmutableCollection templates #40

Merged

Revert "Group test input and output"

95512ac

This reverts commit bf8458f.

rickie approved these changes Feb 23, 2022

View reviewed changes

mussatto approved these changes Feb 23, 2022

View reviewed changes

rickie changed the title ~~Introduce new Time templates for Duration.of methods~~ TimeTemplates: Introduce Duration.of templates Feb 24, 2022

rickie changed the title ~~TimeTemplates: Introduce Duration.of templates~~ TimeTemplates: Introduce Duration.ofX templates Feb 24, 2022

werli approved these changes Feb 24, 2022

View reviewed changes

rickie requested a review from Stephan202 February 28, 2022 11:26

Stephan202 approved these changes Apr 10, 2022

View reviewed changes

Stephan202 added this to the 0.1.0 milestone Apr 10, 2022

Stephan202 merged commit 0d05106 into master Apr 10, 2022

Stephan202 deleted the pdsoels/time-templates-duration-of branch April 10, 2022 14:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`TimeTemplates`: Introduce `Duration.ofX` templates #41

`TimeTemplates`: Introduce `Duration.ofX` templates #41

Badbond commented Feb 18, 2022

rickie left a comment

rickie Feb 22, 2022

rickie Feb 22, 2022

Badbond Feb 22, 2022 •

edited

Loading

rickie Feb 23, 2022

rickie Feb 22, 2022

rickie Feb 22, 2022

Badbond Feb 22, 2022 •

edited

Loading

rickie commented Feb 23, 2022

mussatto left a comment

rickie commented Feb 24, 2022 •

edited

Loading

Badbond commented Feb 24, 2022

werli left a comment

werli Feb 24, 2022

Badbond Feb 24, 2022

werli Feb 24, 2022

Stephan202 left a comment

TimeTemplates: Introduce Duration.ofX templates #41

TimeTemplates: Introduce Duration.ofX templates #41

Conversation

Badbond commented Feb 18, 2022

rickie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Badbond Feb 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Badbond Feb 22, 2022 • edited Loading

Choose a reason for hiding this comment

rickie commented Feb 23, 2022

mussatto left a comment

Choose a reason for hiding this comment

rickie commented Feb 24, 2022 • edited Loading

Badbond commented Feb 24, 2022

werli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stephan202 left a comment

Choose a reason for hiding this comment

`TimeTemplates`: Introduce `Duration.ofX` templates #41

`TimeTemplates`: Introduce `Duration.ofX` templates #41

Badbond Feb 22, 2022 •

edited

Loading

Badbond Feb 22, 2022 •

edited

Loading

rickie commented Feb 24, 2022 •

edited

Loading