[
https://issues.apache.org/jira/browse/GROOVY-12124?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18093143#comment-18093143
]
ASF GitHub Bot commented on GROOVY-12124:
-----------------------------------------
Copilot commented on code in PR #2653:
URL: https://github.com/apache/groovy/pull/2653#discussion_r3510877746
##########
subprojects/groovy-dateutil/src/main/java/org/apache/groovy/dateutil/BaseDuration.java:
##########
@@ -0,0 +1,187 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.groovy.dateutil;
+
+import java.time.temporal.ChronoUnit;
+import java.util.ArrayList;
+import java.util.Calendar;
+import java.util.Date;
+import java.util.List;
+
+/**
+ * Base class for date and time durations (dequirked, {@code
java.util.Date}-flavored).
+ * <p>
+ * This is the B1 prototype: the same value-class model as the legacy
+ * {@code groovy.time.BaseDuration}, but with the historical quirks removed:
+ * <ul>
+ * <li><b>ago / from.now no longer zero the time-of-day</b> for date-based
durations —
+ * the behavior is now uniform across all durations (time is preserved),
so a single
+ * implementation lives here rather than divergent overrides per
subclass.</li>
+ * <li><b>{@link #toMilliseconds()} is deterministic</b> — datum-dependent
amounts (years,
+ * months) use {@link ChronoUnit}'s estimated durations rather than
resolving against
+ * {@code new Date()} ("now"), so the result no longer depends on when
it is called.</li>
+ * </ul>
+ *
+ * @see Duration
+ */
+public abstract class BaseDuration implements Comparable<BaseDuration> {
+ /** Estimated milliseconds in a year (ChronoUnit.YEARS = 365.2425 days). */
+ static final long MILLIS_PER_YEAR =
ChronoUnit.YEARS.getDuration().toMillis();
+ /** Estimated milliseconds in a month (ChronoUnit.MONTHS = 30.436875
days). */
+ static final long MILLIS_PER_MONTH =
ChronoUnit.MONTHS.getDuration().toMillis();
+
+ protected final int years;
+ protected final int months;
+ protected final int days;
+ protected final int hours;
+ protected final int minutes;
+ protected final int seconds;
+ protected final int millis;
+
+ protected BaseDuration(final int years, final int months, final int days,
final int hours, final int minutes, final int seconds, final int millis) {
+ this.years = years;
+ this.months = months;
+ this.days = days;
+ this.hours = hours;
+ this.minutes = minutes;
+ this.seconds = seconds;
+ this.millis = millis;
+ }
+
+ protected BaseDuration(final int days, final int hours, final int minutes,
final int seconds, final int millis) {
+ this(0, 0, days, hours, minutes, seconds, millis);
+ }
+
+ public int getYears() { return this.years; }
+ public int getMonths() { return this.months; }
+ public int getDays() { return this.days; }
+ public int getHours() { return this.hours; }
+ public int getMinutes() { return this.minutes; }
+ public int getSeconds() { return this.seconds; }
+ public int getMillis() { return this.millis; }
+
+ /**
+ * Adds this duration to the supplied date.
+ */
+ public Date plus(final Date date) {
+ final Calendar cal = Calendar.getInstance();
+ cal.setTime(date);
+ cal.add(Calendar.YEAR, this.years);
+ cal.add(Calendar.MONTH, this.months);
+ cal.add(Calendar.DAY_OF_YEAR, this.days);
+ cal.add(Calendar.HOUR_OF_DAY, this.hours);
+ cal.add(Calendar.MINUTE, this.minutes);
+ cal.add(Calendar.SECOND, this.seconds);
+ cal.add(Calendar.MILLISECOND, this.millis);
+ return cal.getTime();
+ }
+
+ /**
+ * Converts this duration to milliseconds.
+ * <p>
+ * DEQUIRKED (B): deterministic. Years and months use {@link ChronoUnit}
estimates
+ * (a year is exactly 12 estimated months, so {@code 1.year ==
12.months}); days and
+ * below are exact assuming 24-hour days. The result never depends on the
current date.
+ */
+ public long toMilliseconds() {
+ return years * MILLIS_PER_YEAR
+ + months * MILLIS_PER_MONTH
+ + days * 86_400_000L
+ + hours * 3_600_000L
+ + minutes * 60_000L
+ + seconds * 1_000L
+ + millis;
+ }
+
+ /**
+ * The date this duration ago.
+ * <p>
+ * DEQUIRKED (A): the time-of-day is preserved (no flooring to midnight),
+ * uniformly for all duration kinds.
+ */
+ public Date getAgo() {
+ final Calendar cal = Calendar.getInstance();
+ cal.add(Calendar.YEAR, -this.years);
+ cal.add(Calendar.MONTH, -this.months);
+ cal.add(Calendar.DAY_OF_YEAR, -this.days);
+ cal.add(Calendar.HOUR_OF_DAY, -this.hours);
+ cal.add(Calendar.MINUTE, -this.minutes);
+ cal.add(Calendar.SECOND, -this.seconds);
+ cal.add(Calendar.MILLISECOND, -this.millis);
+ return cal.getTime();
+ }
+
+ /**
+ * Helper for computing dates relative to the current time.
+ * DEQUIRKED (A): time-of-day preserved.
+ */
+ public From getFrom() {
+ return new From() {
+ @Override
+ public Date getNow() {
+ final Calendar cal = Calendar.getInstance();
+ cal.add(Calendar.YEAR, BaseDuration.this.years);
+ cal.add(Calendar.MONTH, BaseDuration.this.months);
+ cal.add(Calendar.DAY_OF_YEAR, BaseDuration.this.days);
+ cal.add(Calendar.HOUR_OF_DAY, BaseDuration.this.hours);
+ cal.add(Calendar.MINUTE, BaseDuration.this.minutes);
+ cal.add(Calendar.SECOND, BaseDuration.this.seconds);
+ cal.add(Calendar.MILLISECOND, BaseDuration.this.millis);
+ return cal.getTime();
+ }
+ };
+ }
+
+ @Override
+ public int compareTo(BaseDuration other) {
+ return Long.signum(toMilliseconds() - other.toMilliseconds());
+ }
Review Comment:
`compareTo` uses subtraction (`toMilliseconds() - other.toMilliseconds()`),
which can overflow and return the wrong sign for large values, breaking the
`Comparable` contract (inconsistent ordering in sorted collections). Prefer
`Long.compare` to avoid overflow in the comparison step.
##########
subprojects/groovy-dateutil/src/test/groovy/org/apache/groovy/dateutil/TimeCategoryTest.groovy:
##########
@@ -0,0 +1,282 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.groovy.dateutil
+
+import org.junit.jupiter.api.Test
+
+import static java.util.Calendar.DAY_OF_YEAR
+import static java.util.Calendar.MONTH
+import static org.junit.jupiter.api.Assertions.assertTrue
+
+/**
+ * Tests the {@link org.apache.groovy.dateutil.TimeCategory} class, the
dequirked
+ * {@code java.util.Date}-flavored parallel to the legacy {@code
groovy.time.TimeCategory}.
+ * The arithmetic assertions are ported verbatim from the legacy suite; the
final section
+ * exercises the two behaviors that were intentionally changed ("dequirked").
+ */
+class TimeCategoryTest {
+
+ @Test
+ void testDurationArithmeticOnMilliseconds() {
+ use(TimeCategory) {
+ def midnight = new Date(100, 0, 1, 0, 0, 0)
+ def oneSecondPastMidnight = new Date(100, 0, 1, 0, 0, 1)
+ def twoSecondsPastMidnight = new Date(100, 0, 1, 0, 0, 2)
+
+ assert (midnight + 1000.millisecond) == oneSecondPastMidnight
+ assert (midnight + 2000.milliseconds) == twoSecondsPastMidnight
+ assert (twoSecondsPastMidnight - 1000.millisecond) ==
oneSecondPastMidnight
+ assert (twoSecondsPastMidnight - 2000.milliseconds) == midnight
+ }
+ }
+
+ @Test
+ void testDurationArithmeticOnSeconds() {
+ use(TimeCategory) {
+ def midnight = new Date(100, 0, 1, 0, 0, 0)
+ def oneSecondPastMidnight = new Date(100, 0, 1, 0, 0, 1)
+ def twoSecondsPastMidnight = new Date(100, 0, 1, 0, 0, 2)
+
+ assert (midnight + 1.second) == oneSecondPastMidnight
+ assert (midnight + 2.seconds) == twoSecondsPastMidnight
+ assert (twoSecondsPastMidnight - 1.second) == oneSecondPastMidnight
+ assert (twoSecondsPastMidnight - 2.seconds) == midnight
+ }
+ }
+
+ @Test
+ void testDurationArithmeticOnMinutes() {
+ use(TimeCategory) {
+ def midnight = new Date(100, 0, 1, 0, 0, 0)
+ def oneMinutePastMidnight = new Date(100, 0, 1, 0, 1, 0)
+ def twoMinutesPastMidnight = new Date(100, 0, 1, 0, 2, 0)
+
+ assert (midnight + 1.minute) == oneMinutePastMidnight
+ assert (midnight + 2.minutes) == twoMinutesPastMidnight
+ assert (twoMinutesPastMidnight - 60.seconds) ==
oneMinutePastMidnight
+ assert (twoMinutesPastMidnight - 1.minute) == oneMinutePastMidnight
+ assert (twoMinutesPastMidnight - 120.seconds) == midnight
+ assert (twoMinutesPastMidnight - 2.minutes) == midnight
+ }
+ }
+
+ @Test
+ void testDurationArithmeticOnHours() {
+ use(TimeCategory) {
+ def midnight = new Date(100, 0, 1, 0, 0, 0)
+ def oneAM = new Date(100, 0, 1, 1, 0, 0)
+ def twoAM = new Date(100, 0, 1, 2, 0, 0)
+
+ assert (midnight + 1.hour) == oneAM
+ assert (midnight + 2.hours) == twoAM
+ assert (twoAM - 3600.seconds) == oneAM
+ assert (twoAM - 1.hour) == oneAM
+ assert (twoAM - 7200.seconds) == midnight
+ assert (twoAM - 2.hours) == midnight
+ }
+ }
+
+ @Test
+ void testDurationArithmeticOnDays() {
+ use(TimeCategory) {
+ def januaryFirst = new Date(100, 0, 1, 0, 0, 0)
+ def januarySecond = new Date(100, 0, 2, 0, 0, 0)
+ def januaryThird = new Date(100, 0, 3, 0, 0, 0)
+
+ assert (januaryFirst + 1.day) == januarySecond
+ assert (januaryFirst + 2.days) == januaryThird
+ assert (januaryThird - 1.day) == januarySecond
+ assert (januaryThird - 2.days) == januaryFirst
+ }
+ }
+
+ @Test
+ void testDurationArithmeticOnWeeks() {
+ use(TimeCategory) {
+ def firstWeek = new Date(100, 0, 1, 0, 0, 0)
+ def secondWeek = new Date(100, 0, 8, 0, 0, 0)
+ def thirdWeek = new Date(100, 0, 15, 0, 0, 0)
+
+ assert (firstWeek + 1.week) == secondWeek
+ assert (firstWeek + 2.weeks) == thirdWeek
+ assert (thirdWeek - 1.week) == secondWeek
+ assert (thirdWeek - 2.weeks) == firstWeek
+ }
+ }
+
+ @Test
+ void testDurationArithmeticOnMonths() {
+ use(TimeCategory) {
+ def january = new Date(100, 0, 1, 0, 0, 0)
+ def february = new Date(100, 1, 1, 0, 0, 0)
+ def march = new Date(100, 2, 1, 0, 0, 0)
+
+ assert (january + 1.month) == february
+ assert (january + 2.months) == march
+ assert (march - 1.month) == february
+ assert (march - 2.months) == january
+ }
+ }
+
+ @Test
+ void testDurationArithmeticOnYears() {
+ use(TimeCategory) {
+ def firstYear = new Date(100, 0, 1, 0, 0, 0)
+ def secondYear = new Date(101, 0, 1, 0, 0, 0)
+ def thirdYear = new Date(102, 0, 1, 0, 0, 0)
+
+ assert (firstYear + 1.year) == secondYear
+ assert (firstYear + 2.years) == thirdYear
+ assert (thirdYear - 1.year) == secondYear
+ assert (thirdYear - 2.years) == firstYear
+ }
+ }
+
+ @Test
+ void testDateSubtractionOnSeconds() {
+ use(TimeCategory) {
+ def current = new Date(100, 0, 1, 0, 0, 0)
+ def oneSecondLater = new Date(100, 0, 1, 0, 0, 1)
+ def twoSecondsLater = new Date(100, 0, 1, 0, 0, 2)
+
+ assert (oneSecondLater - current).seconds == 1
+ assert (twoSecondsLater - current).seconds == 2
+ }
+ }
+
+ @Test
+ void testDateSubtractionOnMinutes() {
+ use(TimeCategory) {
+ def current = new Date(100, 0, 1, 0, 0, 0)
+ assert (new Date(100, 0, 1, 0, 1, 0) - current).minutes == 1
+ assert (new Date(100, 0, 1, 0, 2, 0) - current).minutes == 2
+ }
+ }
+
+ @Test
+ void testDateSubtractionOnHours() {
+ use(TimeCategory) {
+ def current = new Date(100, 0, 1, 0, 0, 0)
+ assert (new Date(100, 0, 1, 1, 0, 0) - current).hours == 1
+ assert (new Date(100, 0, 1, 2, 0, 0) - current).hours == 2
+ }
+ }
+
+ @Test
+ void testDateSubtractionOnDays() {
+ use(TimeCategory) {
+ def current = new Date(100, 0, 1, 0, 0, 0)
+ assert (new Date(100, 0, 2, 0, 0, 0) - current).days == 1
+ assert (new Date(100, 0, 3, 0, 0, 0) - current).days == 2
+ }
+ }
+
+ @Test
+ void testDateSubtraction_NoYearsOrMonths() {
+ use(TimeCategory) {
+ def result = new Date(102, 0, 1, 0, 0, 0) - new Date(100, 0, 1, 0,
0, 0)
+ // date subtraction does not populate months and years
+ assert result.years == 0
+ assert result.months == 0
+ }
+ }
+
+ @Test
+ void testToStringForNegativeValues() {
+ use(TimeCategory) {
+ def t1 = Calendar.instance.time
+ def t2 = t1 - 4.seconds + 2.milliseconds
+ def t3 = t1 + 4.seconds + 2.milliseconds
+ def t4 = t1 - 4.seconds - 2.milliseconds
+ def t5 = t1 + 4.seconds - 2.milliseconds
+ def t6 = t1 - 2.milliseconds
+ def t7 = t1 + 2.milliseconds
+ assert (t1 - t2).toString() == '3.998 seconds'
+ assert (t1 - t3).toString() == '-4.002 seconds'
+ assert (t1 - t4).toString() == '4.002 seconds'
+ assert (t1 - t5).toString() == '-3.998 seconds'
+ assert (t1 - t6).toString() == '0.002 seconds'
+ assert (t1 - t7).toString() == '-0.002 seconds'
+ }
+ }
+
+ @Test
+ void testToStringForOverflow() {
+ use(TimeCategory) {
+ def t = 800.milliseconds + 300.milliseconds
+ assert t.toString() == '1.100 seconds'
+ }
+ }
+
+ @Test
+ void testZeroDurationFromNowIsNow() {
+ use(TimeCategory) {
+ // Replaces the legacy testDateEquality, whose reproducibility
relied on from.now
+ // flooring to midnight. Dequirked, a zero-length duration from
now is (approximately)
+ // now, with the time-of-day preserved.
+ def reference = System.currentTimeMillis()
+ Date result = 0.days.from.now
+ assert Math.abs(result.time - reference) < 2000
+ }
Review Comment:
This assertion can be flaky under heavy load because it assumes the
`.from.now` call happens within 2 seconds of capturing `reference`. Measuring
`before`/`after` around the call makes the test robust regardless of scheduling
delays.
##########
subprojects/groovy-dateutil/src/test/groovy/org/apache/groovy/dateutil/DurationTest.groovy:
##########
@@ -0,0 +1,171 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.groovy.dateutil
+
+import org.junit.jupiter.api.Test
+
+import java.time.temporal.ChronoUnit
+
+import static java.util.Calendar.DAY_OF_YEAR
+import static java.util.Calendar.MONTH
+import static java.util.Calendar.WEEK_OF_YEAR
+import static org.junit.jupiter.api.Assertions.assertEquals
+import static org.junit.jupiter.api.Assertions.assertNotNull
+import static org.junit.jupiter.api.Assertions.assertTrue
+
+/**
+ * Tests for the dequirked {@code org.apache.groovy.dateutil} Duration
hierarchy.
+ */
+class DurationTest {
+
+ @Test
+ void testFixedDurationArithmetic() {
+ use(TimeCategory) {
+ def oneDay = 2.days - 1.day
+ assert oneDay.toMilliseconds() == (24 * 60 * 60 * 1000)
+
+ oneDay = 2.days - 1.day + 24.hours - 1440.minutes
+ assert oneDay.toMilliseconds() == (24 * 60 * 60 * 1000)
+ }
+ }
+
+ @Test
+ void testDurationToString() {
+ use(TimeCategory) {
+ assert (0.years).toString() == "0"
+ assert (3.years + 8.months + 4.days + 2.hours + 5.minutes +
12.milliseconds).toString() ==
+ "3 years, 8 months, 4 days, 2 hours, 5 minutes, 0.012
seconds"
+ assert (4.days + 2.hours + 5.minutes + 12.milliseconds).toString()
== "4 days, 2 hours, 5 minutes, 0.012 seconds"
+ assert (4.days + 2.hours + 5.minutes).toString() == "4 days, 2
hours, 5 minutes"
+ assert (5.seconds).toString() == "5.000 seconds"
+ assert ((-12).milliseconds).toString() == "-0.012 seconds"
+ assert (5.seconds + 12.milliseconds).toString() == "5.012 seconds"
+ assert (5.seconds + 1012.milliseconds).toString() == "6.012
seconds"
+ }
+ }
+
+ @Test
+ void testDurationComparison() {
+ use(TimeCategory) {
+ assert 1.week < 2.weeks
+ assert 3.weeks <= 4.weeks
+ assert 10.seconds == 10.seconds
+ assert 4.months >= 1.month
+ assert 10.days > 2.days
+ assert 3.months > 2.months
+ assert 1.second < 1.year
+ assert 1.day > 1000.seconds
+ assert 10.weeks < 10.years
+ }
+ }
+
+ @Test
+ void testConstructor() {
+ def duration = new Duration(1, 2, 3, 4, 5)
+ assertEquals(1, duration.days)
+ assertEquals(2, duration.hours)
+ assertEquals(3, duration.minutes)
+ assertEquals(4, duration.seconds)
+ assertEquals(5, duration.millis)
+ }
+
+ @Test
+ void testToMilliseconds() {
+ assertEquals(24 * 60 * 60 * 1000L, new Duration(1, 0, 0, 0,
0).toMilliseconds())
+ def d = new Duration(1, 2, 3, 4, 5)
+ assertEquals(((((1L * 24 + 2) * 60 + 3) * 60 + 4) * 1000) + 5,
d.toMilliseconds())
+ }
+
+ @Test
+ void testDatumDependentToMillisecondsIsDeterministic() { // DEQUIRK B
+ // years/months use fixed ChronoUnit estimates; no dependence on the
current date
+ assertEquals(ChronoUnit.YEARS.duration.toMillis(), new
DatumDependentDuration(1, 0, 0, 0, 0, 0, 0).toMilliseconds())
+ assertEquals(ChronoUnit.MONTHS.duration.toMillis(), new
DatumDependentDuration(0, 1, 0, 0, 0, 0, 0).toMilliseconds())
+ // a year is exactly twelve estimated months
+ assertEquals(new DatumDependentDuration(1, 0, 0, 0, 0, 0,
0).toMilliseconds(),
+ new DatumDependentDuration(0, 12, 0, 0, 0, 0,
0).toMilliseconds())
+ }
+
+ @Test
+ void testPlusDuration() {
+ def result = new Duration(1, 2, 3, 4, 5).plus(new Duration(1, 1, 1, 1,
1))
+ assertEquals(2, result.days); assertEquals(3, result.hours);
assertEquals(4, result.minutes)
+ assertEquals(5, result.seconds); assertEquals(6, result.millis)
+ }
+
+ @Test
+ void testPlusDatumDependentDuration() {
+ def result = new Duration(1, 0, 0, 0, 0).plus(new
DatumDependentDuration(1, 2, 3, 4, 5, 6, 7))
+ assertNotNull(result)
+ assertEquals(1, result.years); assertEquals(2, result.months)
+ }
+
+ @Test
+ void testMinusDatumDependentDuration() {
+ def result = new Duration(10, 5, 30, 20, 100).minus(new
DatumDependentDuration(1, 2, 3, 1, 10, 5, 50))
+ assertEquals(-1, result.years); assertEquals(-2, result.months);
assertEquals(7, result.days)
+ }
+
+ @Test
+ void testGetAgoPreservesTimeOfDay() { // DEQUIRK A
+ def ago = new Duration(1, 0, 0, 0, 0).getAgo()
+ def expected = Calendar.instance
+ expected.add(DAY_OF_YEAR, -1)
+ assertTrue Math.abs(expected.timeInMillis - ago.time) < 2000
+ }
+
+ @Test
+ void testGetFromPreservesTimeOfDay() { // DEQUIRK A
+ def now = new Duration(3, 0, 0, 0, 0).from.now
+ def expected = Calendar.instance
+ expected.add(DAY_OF_YEAR, 3)
+ assertTrue Math.abs(expected.timeInMillis - now.time) < 2000
+ }
+
+ @Test
+ void testMinutesAgo() { // See GROOVY-3687 - was already un-quirky for
time durations
+ use(TimeCategory) {
+ def now = Calendar.instance
+ def before = 10.minutes.ago
+ now.add(Calendar.MINUTE, -11)
+ assertTrue now.timeInMillis < before.time
+ }
+ }
+
+ @Test
+ void testDatumDependantArithmetic() {
Review Comment:
Method name uses the misspelling "Dependant" (vs "Dependent" as in
`DatumDependentDuration`). Renaming keeps terminology consistent across the
codebase and avoids propagating the typo into test reports.
> Modernize the TimeCategory date/time DSL: add a java.time flavor and a
> dequirked java.util.Date flavor
> ------------------------------------------------------------------------------------------------------
>
> Key: GROOVY-12124
> URL: https://issues.apache.org/jira/browse/GROOVY-12124
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Priority: Major
>
> h1. Modernize the TimeCategory date/time DSL
> h2. Summary
> Introduce a {{java.time}}-based parallel to the legacy
> {{groovy.time.TimeCategory}}
> DSL, and give the existing {{java.util.Date}}-based DSL a cleaned-up
> ("dequirked")
> home in the module that already owns Date support. Net result: one familiar
> DSL
> ({{1.hour.ago}}, {{date + 3.months}}, {{2.days.from.now}}) available in two
> flavors —
> modern {{java.time}} output or classic {{java.util.Date}} output — both
> quirk-free.
> h2. Background
> {{groovy.time.TimeCategory}} (a {{use()}} category in *groovy core*) layers
> duration
> arithmetic onto {{java.util.Date}}/{{Calendar}}: {{Integer}} producers
> ({{1.hour}},
> {{2.months}}), operators ({{date + duration}}, {{date - date}}), and
> relative-time
> properties ({{.ago}}, {{.from.now}}). It is backed by the
> {{groovy.time.Duration}}
> hierarchy (Duration / TimeDuration / DatumDependentDuration /
> TimeDatumDependentDuration).
> Two problems motivate this work:
> * *No java.time equivalent.* Modern code uses {{java.time}}, but there is no
> {{1.hour}}/{{2.months}} producer for it. The {{groovy-datetime}} module
> already
> provides all the arithmetic (plus/minus/next/multiply/between/upto…) on
> {{java.time}}
> types — the only gap is the number→amount producers and
> {{.ago}}/{{.from.now}}.
> * *The Date DSL is split and quirky.* All other {{java.util.Date}} DSL methods
> ({{date + int}}, {{date.next()}}, {{date[YEAR]}}, {{clearTime}}, {{format}},
> {{date - date -> int}}) live in the optional *groovy-dateutil* module, while
> TimeCategory lives in core — so half of Date arithmetic is core, half is
> optional,
> and {{date - date}} even resolves differently between them.
> h2. Proposal
> Add two categories and deprecate the legacy one:
> || Class || Module || Output type || Status ||
> | {{org.apache.groovy.datetime.TimeCategory}} | groovy-datetime |
> {{java.time}} (Duration/Period, LocalDate/LocalDateTime) | new |
> | {{org.apache.groovy.dateutil.TimeCategory}} | groovy-dateutil |
> {{java.util.Date}} | new (dequirked) |
> | {{groovy.time.TimeCategory}} (+ Duration hierarchy) | core |
> {{java.util.Date}} | deprecated, frozen |
> The two new categories share one DSL surface and one semantics spec; only the
> terminal
> return types differ. The legacy class stays byte-for-byte behavior-compatible
> (frozen),
> so existing users are unaffected until they choose to migrate.
> h3. Producer mapping (both flavors)
> * {{seconds, minutes, hours, millis, nanos}} → {{java.time.Duration}}
> (datetime) /
> {{TimeDuration}} (dateutil)
> * {{days, weeks, months, years}} → {{java.time.Period}} (datetime) /
> {{Duration}}/{{DatumDependentDuration}} (dateutil)
> * {{.ago}} / {{.from.now}} → {{LocalDate}}/{{LocalDateTime}} (datetime) /
> {{java.util.Date}} (dateutil)
> Note: {{java.time}} deliberately keeps date-based ({{Period}}) and time-based
> ({{Duration}}) amounts separate, so {{date + 2.months + 3.hours}} works by
> left-associative chaining rather than a combined amount type.
> h2. Design decisions
> * *(proposed, pending team review)* The dateutil flavor uses its *own*
> dequirked value
> classes (option "B1"), not {{java.time}} internally (option "B2").
> Rationale: B1 keeps
> the DSL return shape ({{.seconds}}/{{.days}} component accessors, bespoke
> {{toString}},
> the type lattice), so existing tests and user code port ~1:1; it is fully
> self-contained
> within groovy-dateutil (no dependency on groovy-datetime being on the
> classpath — matching
> today, where the Duration classes rely on no external extension methods).
> B2 would change
> component-accessor and {{toString}} semantics and require either
> duplicating datetime's
> operator DGM or a new dateutil→datetime module dependency.
> * *(proposed, pending team review)* Deprecate {{groovy.time.TimeCategory}}
> with a removal
> trajectory (future major). The dateutil copy gives Date users a
> non-deprecated home, so
> the deprecation is not coercive toward java.time.
> * Keep the legacy {{groovy.time.Duration}} value classes exactly as-is (do
> not move; the
> dequirked classes are new copies in {{org.apache.groovy.dateutil}} — no
> split package).
> * The legacy class is *not* a forwarder to the new one (behavior differs); it
> stays intact.
> h2. Quirks removed in the new flavors
> || # || Legacy behavior || New behavior ||
> | A | {{.ago}}/{{.from.now}} floor to midnight for day/month/year durations
> but keep time for hours/min/sec (inconsistent) | Time-of-day preserved
> uniformly (datetime: {{Period}}→{{LocalDate}},
> {{Duration}}→{{LocalDateTime}}) |
> | B | {{DatumDependentDuration.toMilliseconds()}} resolves against {{new
> Date()}} — nondeterministic | Deterministic: uses {{ChronoUnit}} estimates
> ({{1.year == 12.months}} still holds exactly); {{java.time}} {{Period}}
> simply has no {{toMillis}} |
> | C | Four-class Duration lattice incl. {{TimeDatumDependentDuration}} |
> Retained in dateutil (B1); not needed in datetime (chaining) |
> | D | {{Duration}} = 24h/day for {{toMillis}} but DST-aware when added |
> Documented/consistent |
> | E | {{getTimeZone}} (already {{@Deprecated}}), {{getDaylightSavingsOffset}}
> | Dropped from new flavors (superseded by zone-aware java.time) |
> Side effect of A+B: {{getAgo}}/{{getFrom}}/{{toMilliseconds}} collapse to
> single
> implementations on {{BaseDuration}}, removing the per-subclass overrides
> (~40% less
> duplication in the value classes).
> h2. Prototype validation
> A working B1 prototype of the dateutil flavor (six classes) was built and
> exercised
> against a port of {{TimeCategoryTest}}:
> * All arithmetic/{{toString}}/comparison assertions ported with *only* the
> {{use()}}/import
> target changed — no assertion edits.
> * Three added assertions demonstrate the dequirks; the same probes run
> against the legacy
> class confirm the delta (legacy {{3.days.ago}} → {{00:00:00}};
> {{1.month.ago}} off by the
> millis-since-midnight; {{5.months.toMilliseconds()}} varies with the
> current date).
> * Only the value-class suites ({{DurationTest}},
> {{DatumDependentDurationTest}}) need edits,
> confined to the ~6 methods that assert the midnight-flooring /
> {{now}}-relative behavior.
> h2. Scope / tasks
> * [ ] {{org.apache.groovy.datetime.TimeCategory}} + tests + spec section
> (_working-with-datetime-types_)
> * [ ] {{org.apache.groovy.dateutil.TimeCategory}} (B1, dequirked) + value
> classes + tests + spec section
> * [ ] Deprecate {{groovy.time.TimeCategory}} (Javadoc {{@deprecated}}
> pointing to both replacements)
> * [ ] Reconcile/document the {{date - date}} collision (int days vs Duration)
> within groovy-dateutil
> * [ ] Add {{Long}}/{{nanos}} support (enhancement over the Integer-only
> legacy)
> * [ ] Changelog + docgenerator entries
> h2. Open questions (pending team review)
> * B1 vs B2 for the dateutil flavor (recommendation: B1).
> * Commit to eventual removal of {{groovy.time.TimeCategory}} (hard
> {{@Deprecated}}), or
> soft-deprecate in docs only?
> * Category name: reuse the simple name {{TimeCategory}} in both new packages
> (symmetry, but
> dual-import hazard) vs a distinct name for the datetime flavor.
> * Global DGM producers (static-compilation friendly) in addition to the
> {{use()}} category —
> now, later, or never?
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
