search

ThreeTen / threeten

This project was the home of code used to develop a modern date and time library for JDK8. Development has moved to OpenJDK and a separate backport project, threetenbp.
http://threeten.github.io/
191 stars 37 forks source link

Better return type for TemporalField resolve #336

Closed jodastephen closed 10 years ago

jodastephen commented 10 years ago

TemporalField.resolve() currently returns ChronoLocalDate, which is limiting and ties the temporal API more closely to the chrono API than necessary.

Return TemporalAccessor instead and handle a few more types.

jodastephen commented 10 years ago 
# HG changeset patch
# User scolebourne
# Date 1379080138 -3600
# Node ID bcb0dac09d9d0affc0287ebde4b7bad12e7552b7
# Parent  873b257445bb985a884201783a9e3ddcf7d035e2
Better return type for TemporalField resolve

Allow resolve method to return more than just ChronoLocalDate
Fixes #336

diff --git a/src/share/classes/java/time/format/Parsed.java b/src/share/classes/java/time/format/Parsed.java
--- a/src/share/classes/java/time/format/Parsed.java
+++ b/src/share/classes/java/time/format/Parsed.java
@@ -83,6 +83,8 @@
 import java.time.Period;
 import java.time.ZoneId;
 import java.time.chrono.ChronoLocalDate;
+import java.time.chrono.ChronoLocalDateTime;
+import java.time.chrono.ChronoZonedDateTime;
 import java.time.chrono.Chronology;
 import java.time.temporal.ChronoField;
 import java.time.temporal.TemporalAccessor;
@@ -260,11 +262,33 @@
             while (changedCount < 50) {
                 for (Map.Entry<TemporalField, Long> entry : fieldValues.entrySet()) {
                     TemporalField targetField = entry.getKey();
-                    ChronoLocalDate resolvedDate = targetField.resolve(fieldValues, chrono, zone, resolverStyle);
-                    if (resolvedDate != null) {
-                        updateCheckConflict(resolvedDate);
-                        changedCount++;
-                        continue outer;  // have to restart to avoid concurrent modification
+                    TemporalAccessor resolvedObject = targetField.resolve(fieldValues, chrono, zone, resolverStyle);
+                    if (resolvedObject != null) {
+                        if (resolvedObject instanceof ChronoZonedDateTime) {
+                            ChronoZonedDateTime czdt = (ChronoZonedDateTime) resolvedObject;
+                            if (zone.equals(czdt.getZone()) == false) {
+                                throw new DateTimeException("ChronoZonedDateTime must use the effective parsed zone: " + zone);
+                            }
+                            resolvedObject = czdt.toLocalDateTime();
+                        }
+                        if (resolvedObject instanceof ChronoLocalDateTime) {
+                            ChronoLocalDateTime cldt = (ChronoLocalDateTime) resolvedObject;
+                            updateCheckConflict(cldt.toLocalTime(), Period.ZERO);
+                            updateCheckConflict(cldt.toLocalDate());
+                            changedCount++;
+                            continue outer;  // have to restart to avoid concurrent modification
+                        }
+                        if (resolvedObject instanceof ChronoLocalDate) {
+                            updateCheckConflict((ChronoLocalDate) resolvedObject);
+                            changedCount++;
+                            continue outer;  // have to restart to avoid concurrent modification
+                        }
+                        if (resolvedObject instanceof LocalTime) {
+                            updateCheckConflict((LocalTime) resolvedObject, Period.ZERO);
+                            changedCount++;
+                            continue outer;  // have to restart to avoid concurrent modification
+                        }
+                        throw new DateTimeException("Method resolveFields() can only return ChronoLocalDate");
                     } else if (fieldValues.containsKey(targetField) == false) {
                         changedCount++;
                         continue outer;  // have to restart to avoid concurrent modification
@@ -302,7 +326,10 @@
             if (cld != null && date.equals(cld) == false) {
                 throw new DateTimeException("Conflict found: Fields resolved to two different dates: " + date + " " + cld);
             }
-        } else {
+        } else if (cld != null) {
+            if (chrono.equals(cld.getChronology()) == false) {
+                throw new DateTimeException("ChronoLocalDate must use the effective parsed chronology: " + chrono);
+            }
             date = cld;
         }
     }
diff --git a/src/share/classes/java/time/temporal/TemporalField.java b/src/share/classes/java/time/temporal/TemporalField.java
--- a/src/share/classes/java/time/temporal/TemporalField.java
+++ b/src/share/classes/java/time/temporal/TemporalField.java
@@ -63,7 +63,6 @@

 import java.time.DateTimeException;
 import java.time.ZoneId;
-import java.time.chrono.ChronoLocalDate;
 import java.time.chrono.Chronology;
 import java.time.format.ResolverStyle;
 import java.util.Locale;
@@ -350,6 +349,10 @@
      * be acceptable for the date fields to be resolved into other {@code ChronoField}
      * instances that can produce a date, such as {@code EPOCH_DAY}.
      * <p>
+     * Not all {@code TemporalAccessor} implementations are accepted as return values.
+     * Implementations must accept {@code ChronoLocalDate}, {@code ChronoLocalDateTime},
+     * {@code ChronoZonedDateTime} and {@code LocalTime}.
+     * <p>
      * The zone is not normally required for resolution, but is provided for completeness.
      * <p>
      * The default implementation must return null.
@@ -358,13 +361,13 @@
      * @param chronology  the effective chronology, not null
      * @param zone  the effective zone, not null
      * @param resolverStyle  the requested type of resolve, not null
-     * @return the resolved date; null if resolving only changed the map,
-     *  or no resolve occurred
+     * @return the resolved temporal object; null if resolving only
+     *  changed the map, or no resolve occurred
      * @throws ArithmeticException if numeric overflow occurs
      * @throws DateTimeException if resolving results in an error. This must not be thrown
      *  by querying a field on the temporal without first checking if it is supported
      */
-    default ChronoLocalDate resolve(
+    default TemporalAccessor resolve(
             Map<TemporalField, Long> fieldValues, Chronology chronology,
             ZoneId zone, ResolverStyle resolverStyle) {
         return null;
diff --git a/test/java/time/tck/java/time/format/TCKDateTimeParseResolver.java b/test/java/time/tck/java/time/format/TCKDateTimeParseResolver.java
--- a/test/java/time/tck/java/time/format/TCKDateTimeParseResolver.java
+++ b/test/java/time/tck/java/time/format/TCKDateTimeParseResolver.java
@@ -90,20 +90,33 @@
 import static java.time.temporal.ChronoField.SECOND_OF_MINUTE;
 import static java.time.temporal.ChronoField.YEAR;
 import static java.time.temporal.ChronoField.YEAR_OF_ERA;
+import static java.time.temporal.ChronoUnit.DAYS;
+import static java.time.temporal.ChronoUnit.FOREVER;
+import static java.time.temporal.ChronoUnit.NANOS;
 import static org.testng.Assert.assertEquals;
 import static org.testng.Assert.fail;

 import java.time.LocalDate;
+import java.time.LocalDateTime;
 import java.time.LocalTime;
 import java.time.Period;
+import java.time.ZoneId;
+import java.time.ZonedDateTime;
+import java.time.chrono.Chronology;
+import java.time.chrono.ThaiBuddhistChronology;
 import java.time.format.DateTimeFormatter;
 import java.time.format.DateTimeFormatterBuilder;
 import java.time.format.DateTimeParseException;
 import java.time.format.ResolverStyle;
+import java.time.temporal.ChronoUnit;
 import java.time.temporal.IsoFields;
+import java.time.temporal.Temporal;
 import java.time.temporal.TemporalAccessor;
 import java.time.temporal.TemporalField;
 import java.time.temporal.TemporalQuery;
+import java.time.temporal.TemporalUnit;
+import java.time.temporal.ValueRange;
+import java.util.Map;

 import org.testng.annotations.DataProvider;
 import org.testng.annotations.Test;
@@ -872,4 +885,210 @@
         }
     }

+    //-----------------------------------------------------------------------
+    @Test
+    public void test_fieldResolvesToLocalTime() {
+        TemporalField field = new TemporalField() {
+            @Override
+            public TemporalUnit getBaseUnit() {
+                throw new UnsupportedOperationException();
+            }
+            @Override
+            public TemporalUnit getRangeUnit() {
+                throw new UnsupportedOperationException();
+            }
+            @Override
+            public ValueRange range() {
+                throw new UnsupportedOperationException();
+            }
+            @Override
+            public boolean isDateBased() {
+                throw new UnsupportedOperationException();
+            }
+            @Override
+            public boolean isTimeBased() {
+                throw new UnsupportedOperationException();
+            }
+            @Override
+            public boolean isSupportedBy(TemporalAccessor temporal) {
+                throw new UnsupportedOperationException();
+            }
+            @Override
+            public ValueRange rangeRefinedBy(TemporalAccessor temporal) {
+                throw new UnsupportedOperationException();
+            }
+            @Override
+            public long getFrom(TemporalAccessor temporal) {
+                throw new UnsupportedOperationException();
+            }
+            @Override
+            public <R extends Temporal> R adjustInto(R temporal, long newValue) {
+                throw new UnsupportedOperationException();
+            }
+            @Override
+            public TemporalAccessor resolve(
+                    Map<TemporalField, Long> fieldValues, Chronology chronology,
+                    ZoneId zone, ResolverStyle resolverStyle) {
+                return LocalTime.MIDNIGHT.plusNanos(fieldValues.remove(this));
+            }
+        };
+        DateTimeFormatter f = new DateTimeFormatterBuilder().appendValue(field).toFormatter();
+        TemporalAccessor accessor = f.parse("1234567890");
+        assertEquals(accessor.query(TemporalQuery.localDate()), null);
+        assertEquals(accessor.query(TemporalQuery.localTime()), LocalTime.of(0, 0, 1, 234_567_890));
+    }
+
+    @Test
+    public void test_fieldResolvesToChronoLocalDateTime() {
+        TemporalField field = new TemporalField() {
+            @Override
+            public TemporalUnit getBaseUnit() {
+                throw new UnsupportedOperationException();
+            }
+            @Override
+            public TemporalUnit getRangeUnit() {
+                throw new UnsupportedOperationException();
+            }
+            @Override
+            public ValueRange range() {
+                throw new UnsupportedOperationException();
+            }
+            @Override
+            public boolean isDateBased() {
+                throw new UnsupportedOperationException();
+            }
+            @Override
+            public boolean isTimeBased() {
+                throw new UnsupportedOperationException();
+            }
+            @Override
+            public boolean isSupportedBy(TemporalAccessor temporal) {
+                throw new UnsupportedOperationException();
+            }
+            @Override
+            public ValueRange rangeRefinedBy(TemporalAccessor temporal) {
+                throw new UnsupportedOperationException();
+            }
+            @Override
+            public long getFrom(TemporalAccessor temporal) {
+                throw new UnsupportedOperationException();
+            }
+            @Override
+            public <R extends Temporal> R adjustInto(R temporal, long newValue) {
+                throw new UnsupportedOperationException();
+            }
+            @Override
+            public TemporalAccessor resolve(
+                    Map<TemporalField, Long> fieldValues, Chronology chronology,
+                    ZoneId zone, ResolverStyle resolverStyle) {
+                fieldValues.remove(this);
+                return LocalDateTime.of(2010, 6, 30, 12, 30);
+            }
+        };
+        DateTimeFormatter f = new DateTimeFormatterBuilder().appendValue(field).toFormatter();
+        TemporalAccessor accessor = f.parse("1234567890");
+        assertEquals(accessor.query(TemporalQuery.localDate()), LocalDate.of(2010, 6, 30));
+        assertEquals(accessor.query(TemporalQuery.localTime()), LocalTime.of(12, 30));
+    }
+
+    @Test(expectedExceptions = DateTimeParseException.class)
+    public void test_fieldResolvesWrongChrono() {
+        TemporalField field = new TemporalField() {
+            @Override
+            public TemporalUnit getBaseUnit() {
+                throw new UnsupportedOperationException();
+            }
+            @Override
+            public TemporalUnit getRangeUnit() {
+                throw new UnsupportedOperationException();
+            }
+            @Override
+            public ValueRange range() {
+                throw new UnsupportedOperationException();
+            }
+            @Override
+            public boolean isDateBased() {
+                throw new UnsupportedOperationException();
+            }
+            @Override
+            public boolean isTimeBased() {
+                throw new UnsupportedOperationException();
+            }
+            @Override
+            public boolean isSupportedBy(TemporalAccessor temporal) {
+                throw new UnsupportedOperationException();
+            }
+            @Override
+            public ValueRange rangeRefinedBy(TemporalAccessor temporal) {
+                throw new UnsupportedOperationException();
+            }
+            @Override
+            public long getFrom(TemporalAccessor temporal) {
+                throw new UnsupportedOperationException();
+            }
+            @Override
+            public <R extends Temporal> R adjustInto(R temporal, long newValue) {
+                throw new UnsupportedOperationException();
+            }
+            @Override
+            public TemporalAccessor resolve(
+                    Map<TemporalField, Long> fieldValues, Chronology chronology,
+                    ZoneId zone, ResolverStyle resolverStyle) {
+                return ThaiBuddhistChronology.INSTANCE.dateNow();
+            }
+        };
+        DateTimeFormatter f = new DateTimeFormatterBuilder().appendValue(field).toFormatter();
+        f.parse("1234567890");
+    }
+
+    @Test(expectedExceptions = DateTimeParseException.class)
+    public void test_fieldResolvesWrongZone() {
+        TemporalField field = new TemporalField() {
+            @Override
+            public TemporalUnit getBaseUnit() {
+                throw new UnsupportedOperationException();
+            }
+            @Override
+            public TemporalUnit getRangeUnit() {
+                throw new UnsupportedOperationException();
+            }
+            @Override
+            public ValueRange range() {
+                throw new UnsupportedOperationException();
+            }
+            @Override
+            public boolean isDateBased() {
+                throw new UnsupportedOperationException();
+            }
+            @Override
+            public boolean isTimeBased() {
+                throw new UnsupportedOperationException();
+            }
+            @Override
+            public boolean isSupportedBy(TemporalAccessor temporal) {
+                throw new UnsupportedOperationException();
+            }
+            @Override
+            public ValueRange rangeRefinedBy(TemporalAccessor temporal) {
+                throw new UnsupportedOperationException();
+            }
+            @Override
+            public long getFrom(TemporalAccessor temporal) {
+                throw new UnsupportedOperationException();
+            }
+            @Override
+            public <R extends Temporal> R adjustInto(R temporal, long newValue) {
+                throw new UnsupportedOperationException();
+            }
+            @Override
+            public TemporalAccessor resolve(
+                    Map<TemporalField, Long> fieldValues, Chronology chronology,
+                    ZoneId zone, ResolverStyle resolverStyle) {
+                return ZonedDateTime.of(2010, 6, 30, 12, 30, 0, 0, ZoneId.of("Europe/Paris"));
+            }
+        };
+        DateTimeFormatter f = new DateTimeFormatterBuilder().appendValue(field).toFormatter().withZone(ZoneId.of("Europe/London"));
+        f.parse("1234567890");
+    }
+
 }
RogerRiggs commented 10 years ago

Can you expand on the explanation of the change. I understand conceptually resolving a field may affect the date or time or Chronology; is there more to it than that?

"+ throw new DateTimeException("Method resolveFields() can only return ChronoLocalDate");" This exception text is inaccurate now.

jodastephen commented 10 years ago

In an ideal world, the temporal interfaces should not refer directly to the chrono interfaces. This place was one unecessary link. In generalising the link, it allows resolve implementations to be more flexible and have better performance, both of which seem like good things.

I'll fix the error message if you give me permission to push.

RogerRiggs commented 10 years ago

Looks fine. Thanks

RogerRiggs commented 10 years ago

JBS Mirror issue is 8024834

jodastephen commented 10 years ago

http://hg.openjdk.java.net/threeten/threeten/jdk/rev/b7f9218d0f37