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.

Sign up for GitHub

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

TimeTemplates: Introduce Duration.ofX templates #41

Merged
merged 7 commits into from
Apr 10, 2022
Merged

TimeTemplates: Introduce Duration.ofX templates #41

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
af9fb6b
Add Duration.ofMillis template
Badbond Feb 18, 2022
4420491
Add remaining Duration.ofX methods
Badbond Feb 18, 2022
0b62344
Add static import test
Badbond Feb 18, 2022
27813a8
Add ChronoUnit.DAYS static import to exemptions
Badbond Feb 21, 2022
9330174
Add JavaDoc and order lexicographically
Badbond Feb 22, 2022
bf8458f
Group test input and output
Badbond Feb 22, 2022
95512ac
Revert "Group test input and output"
Badbond Feb 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 75 additions & 2 deletions ...r-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/TimeTemplates.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import java.time.chrono.ChronoLocalDate;
import java.time.chrono.ChronoLocalDateTime;
import java.time.chrono.ChronoZonedDateTime;
import java.time.temporal.ChronoUnit;
import java.time.temporal.TemporalUnit;

/** Refaster templates related to expressions dealing with time. */
Expand Down Expand Up @@ -310,8 +311,80 @@ Duration after() {
}
}

static final class DurationOfNanos {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 😄.

Copy link
Member Author

@Badbond Badbond Feb 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 😄.

@BeforeTemplate
Duration before(long amount) {
return Duration.of(amount, ChronoUnit.NANOS);
}

@AfterTemplate
Duration after(long amount) {
return Duration.ofNanos(amount);
}
}

static final class DurationOfMillis {
@BeforeTemplate
Duration before(long amount) {
return Duration.of(amount, ChronoUnit.MILLIS);
}

@AfterTemplate
Duration after(long amount) {
return Duration.ofMillis(amount);
}
}

static final class DurationOfSeconds {
@BeforeTemplate
Duration before(long amount) {
return Duration.of(amount, ChronoUnit.SECONDS);
}

@AfterTemplate
Duration after(long amount) {
return Duration.ofSeconds(amount);
}
}

static final class DurationOfMinutes {
@BeforeTemplate
Duration before(long amount) {
return Duration.of(amount, ChronoUnit.MINUTES);
}

@AfterTemplate
Duration after(long amount) {
return Duration.ofMinutes(amount);
}
}

static final class DurationOfHours {
@BeforeTemplate
Duration before(long amount) {
return Duration.of(amount, ChronoUnit.HOURS);
}

@AfterTemplate
Duration after(long amount) {
return Duration.ofHours(amount);
}
}

static final class DurationOfDays {
@BeforeTemplate
Duration before(long amount) {
return Duration.of(amount, ChronoUnit.DAYS);
}

@AfterTemplate
Duration after(long amount) {
return Duration.ofDays(amount);
}
}

/**
* Don't unnecessarily convert two and from milliseconds. (This way nanosecond precision is
* Don't unnecessarily convert to and from milliseconds. (This way nanosecond precision is
* retained.)
*
* <p><strong>Warning:</strong> this rewrite rule increases precision!
Expand All @@ -329,7 +402,7 @@ Duration after(Instant a, Instant b) {
}

/**
* Don't unnecessarily convert two and from milliseconds. (This way nanosecond precision is
* Don't unnecessarily convert to and from milliseconds. (This way nanosecond precision is
* retained.)
*
* <p><strong>Warning:</strong> this rewrite rule increases precision!
Expand Down
32 changes: 31 additions & 1 deletion ...contrib/src/test/resources/tech/picnic/errorprone/bugpatterns/TimeTemplatesTestInput.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package tech.picnic.errorprone.bugpatterns;

import static java.time.temporal.ChronoUnit.DAYS;
Badbond marked this conversation as resolved.
Show resolved Hide resolved

import com.google.common.collect.ImmutableSet;
import java.time.Clock;
import java.time.Duration;
Expand All @@ -17,7 +19,7 @@
final class TimeTemplatesTest implements RefasterTemplateTestCase {
@Override
public ImmutableSet<?> elidedTypesAndStaticImports() {
return ImmutableSet.of(ChronoUnit.class);
return ImmutableSet.of(ChronoUnit.class, DAYS);
}

Instant testClockInstant() {
Expand Down Expand Up @@ -128,6 +130,34 @@ ImmutableSet<Duration> testZeroDuration() {
Duration.of(0, ChronoUnit.MILLIS));
}

Duration testDurationOfNanos() {
return Duration.of(1, ChronoUnit.NANOS);
}

Duration testDurationOfMillis() {
return Duration.of(1, ChronoUnit.MILLIS);
}

Duration testDurationOfSeconds() {
return Duration.of(1, ChronoUnit.SECONDS);
}

Duration testDurationOfMinutes() {
return Duration.of(1, ChronoUnit.MINUTES);
}

Duration testDurationOfHours() {
return Duration.of(1, ChronoUnit.HOURS);
}

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

Duration testDurationOfDaysStaticImport() {
Copy link
Member

Choose a reason for hiding this comment

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

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 😄.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@Badbond Badbond Feb 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

return Duration.of(1, DAYS);
}

Duration testDurationBetweenInstants() {
return Duration.ofMillis(Instant.MAX.toEpochMilli() - Instant.MIN.toEpochMilli());
}
Expand Down
32 changes: 31 additions & 1 deletion ...ontrib/src/test/resources/tech/picnic/errorprone/bugpatterns/TimeTemplatesTestOutput.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package tech.picnic.errorprone.bugpatterns;

import static java.time.temporal.ChronoUnit.DAYS;

import com.google.common.collect.ImmutableSet;
import java.time.Clock;
import java.time.Duration;
Expand All @@ -17,7 +19,7 @@
final class TimeTemplatesTest implements RefasterTemplateTestCase {
@Override
public ImmutableSet<?> elidedTypesAndStaticImports() {
return ImmutableSet.of(ChronoUnit.class);
return ImmutableSet.of(ChronoUnit.class, DAYS);
}

Instant testClockInstant() {
Expand Down Expand Up @@ -117,6 +119,34 @@ ImmutableSet<Duration> testZeroDuration() {
Duration.ZERO);
}

Duration testDurationOfNanos() {
return Duration.ofNanos(1);
}

Duration testDurationOfMillis() {
return Duration.ofMillis(1);
}

Duration testDurationOfSeconds() {
return Duration.ofSeconds(1);
}

Duration testDurationOfMinutes() {
return Duration.ofMinutes(1);
}

Duration testDurationOfHours() {
return Duration.ofHours(1);
}

Duration testDurationOfDays() {
return Duration.ofDays(1);
}

Duration testDurationOfDaysStaticImport() {
return Duration.ofDays(1);
}

Duration testDurationBetweenInstants() {
return Duration.between(Instant.MIN, Instant.MAX);
}
Expand Down