Skip to content
This repository has been archived by the owner. It is now read-only.

Change Chronology to be an interface #321

Closed
jodastephen opened this issue Jun 24, 2013 · 13 comments

Comments

2 participants
@jodastephen
Copy link
Member

commented Jun 24, 2013

Chronology is currently a class, but might be better as an interface.

While writing tests I found myself writing JapaneseChronology.from(localDate), expecting it to return a JapaneseDate. It didn't because the from method is on Chronology and intended for use in extracting the chronology from a temporal object.

The other static methods, of(String), ofLocale(Locale) and getAvailableChronologies() all suffer from the same static inheritance problem.

Using an interface fits with the package, where other Chrono classes are interfaces. It also allows the same static methods to not be inherited by subclasses, avoiding errors. Making the change would require a ChronologyImpl package scoped class to handle the current behaviour.

@RogerRiggs

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2013

Perhaps using JapaneseDate.from(localDate) would be more natural and gives the desired result.
What impact is there on serialization?

@jodastephen

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2013

I played with this for an hour or so. I think the right approach would be as follows:

  • Make Chronology into an interface
  • Make resolveDate abstract, and other methods on Chronology defaults
  • Add new public AbstractChronology class, with the resolveDate defined here
  • Define generics as AbstractChronology<D extends ChronoLocalDate>, which allows facory methods like date(..) to no longer need overriding in classes like MingoChronology
  • Remove generics from ChronoLocalDate, otherwise there is lots of stupid casting
  • Define serialization as only applying to AbstractChronology

I think the end result would be neater, with the extra abstract class simplifying external chronology implementations.

The smaller change would make AbstractChronology non-public, but I suspect thats a missed opportunity.

@RogerRiggs

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2013

That's a very large change just to make a few methods not be inherited.

  • I would avoid adding a public class for AbstractChronology; it is extra surface area to test and would likely lead to other refactoring to support other external Chronologies.
  • Some of the date(...) classes still need to be overridden to create the MinguoDate instances
  • I've been round and round about the generics on ChronoLocalDate and the wildcard generics there are still the best option to accurately reflect consistently generics and wildcard types.

Are there other benefits besides the original observation?

@jodastephen

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2013

This minor patch moves the package scoped methods on Chronology to the impl classes. This should be committed whatever we do with the rest of the issue.

# HG changeset patch
# User scolebourne
# Date 1372347579 -3600
# Node ID 3fca04188e348e351268d8e59ab7b3dd7e20beb8
# Parent  73a95812438c7f7f95eb7b4d26ac74f98474edaa
Move ensureChrono() methods to be static on impl classes
This is a better place for the code
Better casting has also been added
See #321

diff --git a/src/share/classes/java/time/chrono/ChronoDateImpl.java b/src/share/classes/java/time/chrono/ChronoDateImpl.java
--- a/src/share/classes/java/time/chrono/ChronoDateImpl.java
+++ b/src/share/classes/java/time/chrono/ChronoDateImpl.java
@@ -147,6 +147,25 @@
     private static final long serialVersionUID = 6282433883239719096L;

     /**
+     * Casts the {@code Temporal} to {@code ChronoLocalDate} ensuring it bas the specified chronology.
+     *
+     * @param chrono  the chronology to check for, not null
+     * @param temporal  a date-time to cast, not null
+     * @return the date-time checked and cast to {@code ChronoLocalDate}, not null
+     * @throws ClassCastException if the date-time cannot be cast to ChronoLocalDate
+     *  or the chronology is not equal this Chronology
+     */
+    static <D extends ChronoLocalDate<D>> D ensureValid(Chronology chrono, Temporal temporal) {
+        @SuppressWarnings("unchecked")
+        D other = (D) temporal;
+        if (chrono.equals(other.getChronology()) == false) {
+            throw new ClassCastException("Chronology mismatch, expected: " + chrono.getId() + ", actual: " + other.getChronology().getId());
+        }
+        return other;
+    }
+
+    //-----------------------------------------------------------------------
+    /**
      * Creates an instance.
      */
     ChronoDateImpl() {
diff --git a/src/share/classes/java/time/chrono/ChronoLocalDate.java b/src/share/classes/java/time/chrono/ChronoLocalDate.java
--- a/src/share/classes/java/time/chrono/ChronoLocalDate.java
+++ b/src/share/classes/java/time/chrono/ChronoLocalDate.java
@@ -384,7 +384,7 @@
      */
     @Override
     default D with(TemporalAdjuster adjuster) {
-        return (D) getChronology().ensureChronoLocalDate(Temporal.super.with(adjuster));
+        return ChronoDateImpl.ensureValid(getChronology(), Temporal.super.with(adjuster));
     }

     /**
@@ -398,7 +398,7 @@
         if (field instanceof ChronoField) {
             throw new UnsupportedTemporalTypeException("Unsupported field: " + field);
         }
-        return (D) getChronology().ensureChronoLocalDate(field.adjustInto(this, newValue));
+        return ChronoDateImpl.ensureValid(getChronology(), field.adjustInto(this, newValue));
     }

     /**
@@ -408,7 +408,7 @@
      */
     @Override
     default D plus(TemporalAmount amount) {
-        return (D) getChronology().ensureChronoLocalDate(Temporal.super.plus(amount));
+        return ChronoDateImpl.ensureValid(getChronology(), Temporal.super.plus(amount));
     }

     /**
@@ -421,7 +421,7 @@
         if (unit instanceof ChronoUnit) {
             throw new UnsupportedTemporalTypeException("Unsupported unit: " + unit);
         }
-        return (D) getChronology().ensureChronoLocalDate(unit.addTo(this, amountToAdd));
+        return ChronoDateImpl.ensureValid(getChronology(), unit.addTo(this, amountToAdd));
     }

     /**
@@ -431,7 +431,7 @@
      */
     @Override
     default D minus(TemporalAmount amount) {
-        return (D) getChronology().ensureChronoLocalDate(Temporal.super.minus(amount));
+        return ChronoDateImpl.ensureValid(getChronology(), Temporal.super.minus(amount));
     }

     /**
@@ -442,7 +442,7 @@
      */
     @Override
     default D minus(long amountToSubtract, TemporalUnit unit) {
-        return (D) getChronology().ensureChronoLocalDate(Temporal.super.minus(amountToSubtract, unit));
+        return ChronoDateImpl.ensureValid(getChronology(), Temporal.super.minus(amountToSubtract, unit));
     }

     //-----------------------------------------------------------------------
diff --git a/src/share/classes/java/time/chrono/ChronoLocalDateTime.java b/src/share/classes/java/time/chrono/ChronoLocalDateTime.java
--- a/src/share/classes/java/time/chrono/ChronoLocalDateTime.java
+++ b/src/share/classes/java/time/chrono/ChronoLocalDateTime.java
@@ -203,7 +203,7 @@
      */
     @Override
     default ChronoLocalDateTime<D> with(TemporalAdjuster adjuster) {
-        return (ChronoLocalDateTime<D>)(toLocalDate().getChronology().ensureChronoLocalDateTime(Temporal.super.with(adjuster)));
+        return ChronoLocalDateTimeImpl.ensureValid(toLocalDate().getChronology(), Temporal.super.with(adjuster));
     }

     /**
@@ -221,7 +221,7 @@
      */
     @Override
     default ChronoLocalDateTime<D> plus(TemporalAmount amount) {
-        return (ChronoLocalDateTime<D>)(toLocalDate().getChronology().ensureChronoLocalDateTime(Temporal.super.plus(amount)));
+        return ChronoLocalDateTimeImpl.ensureValid(toLocalDate().getChronology(), Temporal.super.plus(amount));
     }

     /**
@@ -239,7 +239,7 @@
      */
     @Override
     default ChronoLocalDateTime<D> minus(TemporalAmount amount) {
-        return (ChronoLocalDateTime<D>)(toLocalDate().getChronology().ensureChronoLocalDateTime(Temporal.super.minus(amount)));
+        return ChronoLocalDateTimeImpl.ensureValid(toLocalDate().getChronology(), Temporal.super.minus(amount));
     }

     /**
@@ -249,7 +249,7 @@
      */
     @Override
     default ChronoLocalDateTime<D> minus(long amountToSubtract, TemporalUnit unit) {
-        return (ChronoLocalDateTime<D>)(toLocalDate().getChronology().ensureChronoLocalDateTime(Temporal.super.minus(amountToSubtract, unit)));
+        return ChronoLocalDateTimeImpl.ensureValid(toLocalDate().getChronology(), Temporal.super.minus(amountToSubtract, unit));
     }

     //-----------------------------------------------------------------------
diff --git a/src/share/classes/java/time/chrono/ChronoLocalDateTimeImpl.java b/src/share/classes/java/time/chrono/ChronoLocalDateTimeImpl.java
--- a/src/share/classes/java/time/chrono/ChronoLocalDateTimeImpl.java
+++ b/src/share/classes/java/time/chrono/ChronoLocalDateTimeImpl.java
@@ -177,6 +177,25 @@
     }

     /**
+     * Casts the {@code Temporal} to {@code ChronoLocalDateTime} ensuring it bas the specified chronology.
+     *
+     * @param chrono  the chronology to check for, not null
+     * @param temporal   a date-time to cast, not null
+     * @return the date-time checked and cast to {@code ChronoLocalDateTime}, not null
+     * @throws ClassCastException if the date-time cannot be cast to ChronoLocalDateTimeImpl
+     *  or the chronology is not equal this Chronology
+     */
+    static <D extends ChronoLocalDate<D>> ChronoLocalDateTimeImpl<D> ensureValid(Chronology chrono, Temporal temporal) {
+        @SuppressWarnings("unchecked")
+        ChronoLocalDateTimeImpl<D> other = (ChronoLocalDateTimeImpl<D>) temporal;
+        if (chrono.equals(other.toLocalDate().getChronology()) == false) {
+            throw new ClassCastException("Chronology mismatch, required: " + chrono.getId()
+                    + ", actual: " + other.toLocalDate().getChronology().getId());
+        }
+        return other;
+    }
+
+    /**
      * Constructor.
      *
      * @param date  the date part of the date-time, not null
@@ -202,7 +221,7 @@
             return this;
         }
         // Validate that the new Temporal is a ChronoLocalDate (and not something else)
-        D cd = (D) date.getChronology().ensureChronoLocalDate(newDate);
+        D cd = ChronoDateImpl.ensureValid(date.getChronology(), newDate);
         return new ChronoLocalDateTimeImpl<>(cd, newTime);
     }

@@ -264,9 +283,9 @@
         } else if (adjuster instanceof LocalTime) {
             return with(date, (LocalTime) adjuster);
         } else if (adjuster instanceof ChronoLocalDateTimeImpl) {
-            return (ChronoLocalDateTimeImpl<D>)(date.getChronology().ensureChronoLocalDateTime((ChronoLocalDateTimeImpl<?>) adjuster));
+            return ChronoLocalDateTimeImpl.ensureValid(date.getChronology(), (ChronoLocalDateTimeImpl<?>) adjuster);
         }
-        return (ChronoLocalDateTimeImpl<D>)(date.getChronology().ensureChronoLocalDateTime((ChronoLocalDateTimeImpl<?>) adjuster.adjustInto(this)));
+        return ChronoLocalDateTimeImpl.ensureValid(date.getChronology(), (ChronoLocalDateTimeImpl<?>) adjuster.adjustInto(this));
     }

     @Override
@@ -279,7 +298,7 @@
                 return with(date.with(field, newValue), time);
             }
         }
-        return (ChronoLocalDateTimeImpl<D>)(date.getChronology().ensureChronoLocalDateTime(field.adjustInto(this, newValue)));
+        return ChronoLocalDateTimeImpl.ensureValid(date.getChronology(), field.adjustInto(this, newValue));
     }

     //-----------------------------------------------------------------------
@@ -298,7 +317,7 @@
             }
             return with(date.plus(amountToAdd, unit), time);
         }
-        return (ChronoLocalDateTimeImpl<D>)(date.getChronology().ensureChronoLocalDateTime(unit.addTo(this, amountToAdd)));
+        return ChronoLocalDateTimeImpl.ensureValid(date.getChronology(), unit.addTo(this, amountToAdd));
     }

     private ChronoLocalDateTimeImpl<D> plusDays(long days) {
diff --git a/src/share/classes/java/time/chrono/ChronoZonedDateTime.java b/src/share/classes/java/time/chrono/ChronoZonedDateTime.java
--- a/src/share/classes/java/time/chrono/ChronoZonedDateTime.java
+++ b/src/share/classes/java/time/chrono/ChronoZonedDateTime.java
@@ -350,7 +350,7 @@
      */
     @Override
     default ChronoZonedDateTime<D> with(TemporalAdjuster adjuster) {
-        return (ChronoZonedDateTime<D>)(toLocalDate().getChronology().ensureChronoZonedDateTime(Temporal.super.with(adjuster)));
+        return ChronoZonedDateTimeImpl.ensureValid(toLocalDate().getChronology(), Temporal.super.with(adjuster));
     }

     /**
@@ -368,7 +368,7 @@
      */
     @Override
     default ChronoZonedDateTime<D> plus(TemporalAmount amount) {
-        return (ChronoZonedDateTime<D>)(toLocalDate().getChronology().ensureChronoZonedDateTime(Temporal.super.plus(amount)));
+        return ChronoZonedDateTimeImpl.ensureValid(toLocalDate().getChronology(), Temporal.super.plus(amount));
     }

     /**
@@ -386,7 +386,7 @@
      */
     @Override
     default ChronoZonedDateTime<D> minus(TemporalAmount amount) {
-        return (ChronoZonedDateTime<D>)(toLocalDate().getChronology().ensureChronoZonedDateTime(Temporal.super.minus(amount)));
+        return ChronoZonedDateTimeImpl.ensureValid(toLocalDate().getChronology(), Temporal.super.minus(amount));
     }

     /**
@@ -396,7 +396,7 @@
      */
     @Override
     default ChronoZonedDateTime<D> minus(long amountToSubtract, TemporalUnit unit) {
-        return (ChronoZonedDateTime<D>)(toLocalDate().getChronology().ensureChronoZonedDateTime(Temporal.super.minus(amountToSubtract, unit)));
+        return ChronoZonedDateTimeImpl.ensureValid(toLocalDate().getChronology(), Temporal.super.minus(amountToSubtract, unit));
     }

     //-----------------------------------------------------------------------
diff --git a/src/share/classes/java/time/chrono/ChronoZonedDateTimeImpl.java b/src/share/classes/java/time/chrono/ChronoZonedDateTimeImpl.java
--- a/src/share/classes/java/time/chrono/ChronoZonedDateTimeImpl.java
+++ b/src/share/classes/java/time/chrono/ChronoZonedDateTimeImpl.java
@@ -188,6 +188,25 @@
         return (ChronoZonedDateTimeImpl<D>)ofInstant(toLocalDate().getChronology(), instant, zone);
     }

+    /**
+     * Casts the {@code Temporal} to {@code ChronoZonedDateTimeImpl} ensuring it bas the specified chronology.
+     *
+     * @param chrono  the chronology to check for, not null
+     * @param temporal  a date-time to cast, not null
+     * @return the date-time checked and cast to {@code ChronoZonedDateTimeImpl}, not null
+     * @throws ClassCastException if the date-time cannot be cast to ChronoZonedDateTimeImpl
+     *  or the chronology is not equal this Chronology
+     */
+    static <D extends ChronoLocalDate<D>> ChronoZonedDateTimeImpl<D> ensureValid(Chronology chrono, Temporal temporal) {
+        @SuppressWarnings("unchecked")
+        ChronoZonedDateTimeImpl<D> other = (ChronoZonedDateTimeImpl<D>) temporal;
+        if (chrono.equals(other.toLocalDate().getChronology()) == false) {
+            throw new ClassCastException("Chronology mismatch, required: " + chrono.getId()
+                    + ", actual: " + other.toLocalDate().getChronology().getId());
+        }
+        return other;
+    }
+
     //-----------------------------------------------------------------------
     /**
      * Constructor.
@@ -271,7 +290,7 @@
             }
             return ofBest(dateTime.with(field, newValue), zone, offset);
         }
-        return (ChronoZonedDateTime<D>)(toLocalDate().getChronology().ensureChronoZonedDateTime(field.adjustInto(this, newValue)));
+        return ChronoZonedDateTimeImpl.ensureValid(toLocalDate().getChronology(), field.adjustInto(this, newValue));
     }

     //-----------------------------------------------------------------------
@@ -280,7 +299,7 @@
         if (unit instanceof ChronoUnit) {
             return with(dateTime.plus(amountToAdd, unit));
         }
-        return (ChronoZonedDateTime<D>)(toLocalDate().getChronology().ensureChronoZonedDateTime(unit.addTo(this, amountToAdd)));   /// TODO: Generics replacement Risk!
+        return ChronoZonedDateTimeImpl.ensureValid(toLocalDate().getChronology(), unit.addTo(this, amountToAdd));   /// TODO: Generics replacement Risk!
     }

     //-----------------------------------------------------------------------
diff --git a/src/share/classes/java/time/chrono/Chronology.java b/src/share/classes/java/time/chrono/Chronology.java
--- a/src/share/classes/java/time/chrono/Chronology.java
+++ b/src/share/classes/java/time/chrono/Chronology.java
@@ -487,60 +487,6 @@

     //-----------------------------------------------------------------------
     /**
-     * Casts the {@code Temporal} to {@code ChronoLocalDate} with the same chronology.
-     *
-     * @param temporal  a date-time to cast, not null
-     * @return the date-time checked and cast to {@code ChronoLocalDate}, not null
-     * @throws ClassCastException if the date-time cannot be cast to ChronoLocalDate
-     *  or the chronology is not equal this Chronology
-     */
-    ChronoLocalDate<?> ensureChronoLocalDate(Temporal temporal) {
-        @SuppressWarnings("unchecked")
-        ChronoLocalDate<?> other = (ChronoLocalDate<?>) temporal;
-        if (this.equals(other.getChronology()) == false) {
-            throw new ClassCastException("Chronology mismatch, expected: " + getId() + ", actual: " + other.getChronology().getId());
-        }
-        return other;
-    }
-
-    /**
-     * Casts the {@code Temporal} to {@code ChronoLocalDateTime} with the same chronology.
-     *
-     * @param temporal   a date-time to cast, not null
-     * @return the date-time checked and cast to {@code ChronoLocalDateTime}, not null
-     * @throws ClassCastException if the date-time cannot be cast to ChronoLocalDateTimeImpl
-     *  or the chronology is not equal this Chronology
-     */
-    ChronoLocalDateTimeImpl<?> ensureChronoLocalDateTime(Temporal temporal) {
-        @SuppressWarnings("unchecked")
-        ChronoLocalDateTimeImpl<?> other = (ChronoLocalDateTimeImpl<?>) temporal;
-        if (this.equals(other.toLocalDate().getChronology()) == false) {
-            throw new ClassCastException("Chronology mismatch, required: " + getId()
-                    + ", supplied: " + other.toLocalDate().getChronology().getId());
-        }
-        return other;
-    }
-
-    /**
-     * Casts the {@code Temporal} to {@code ChronoZonedDateTimeImpl} with the same chronology.
-     *
-     * @param temporal  a date-time to cast, not null
-     * @return the date-time checked and cast to {@code ChronoZonedDateTimeImpl}, not null
-     * @throws ClassCastException if the date-time cannot be cast to ChronoZonedDateTimeImpl
-     *  or the chronology is not equal this Chronology
-     */
-    ChronoZonedDateTimeImpl<?> ensureChronoZonedDateTime(Temporal temporal) {
-        @SuppressWarnings("unchecked")
-        ChronoZonedDateTimeImpl<?> other = (ChronoZonedDateTimeImpl<?>) temporal;
-        if (this.equals(other.toLocalDate().getChronology()) == false) {
-            throw new ClassCastException("Chronology mismatch, required: " + getId()
-                    + ", supplied: " + other.toLocalDate().getChronology().getId());
-        }
-        return other;
-    }
-
-    //-----------------------------------------------------------------------
-    /**
      * Gets the ID of the chronology.
      * <p>
      * The ID uniquely identifies the {@code Chronology}.
@@ -768,7 +714,7 @@

             } catch (DateTimeException ex1) {
                 @SuppressWarnings("rawtypes")
-                ChronoLocalDateTimeImpl cldt = ensureChronoLocalDateTime(localDateTime(temporal));
+                ChronoLocalDateTimeImpl cldt = ChronoLocalDateTimeImpl.ensureValid(this, localDateTime(temporal));
                 return ChronoZonedDateTimeImpl.ofBest(cldt, zone, null);
             }
         } catch (DateTimeException ex) {
@jodastephen

This comment has been minimized.

@jodastephen

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2013

Patch of the basic proposal added https://gist.github.com/jodastephen/5961060

This involves making Chronology an interface, and adding a public AbstractChronology. If generics are simplified on ChronoLocalDate, then AbstractChronology<D extends ChronoLocalDate> becomes possible to save casting overrides in subclasses.

AbstractChronology has to be public as the resolveDate method is too complex to be a default method on an interface (as we don't have package scoped methods on interfaces yet).

@RogerRiggs

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2013

Finally got back to look at this again with the API freeze deadline looming.
It seems like a reasonable change, it is fairly localized though a number of tests will need to be updated also.

@jodastephen

This comment has been minimized.

Copy link
Member Author

commented Sep 22, 2013

To confirm, we do the Chronology interface+ AbstractChronology public class. Can I make the change on the threeten repo? Is it up to date enough?

@RogerRiggs

This comment has been minimized.

Copy link
Contributor

commented Sep 22, 2013

Hi,

The threaten repository is up to date enough for the change though it is out of date with jdk8-tl to and jdk8.

I would like to transition to working from jdk8-tl as the reviews are easier to merge and the latency will be lower.

Thanks, Roger

On Sep 22, 2013, at 0:27, Stephen Colebourne notifications@github.com wrote:

To confirm, we do the Chronology interface+ AbstractChronology public class. Can I make the change on the threeten repo? Is it up to date enough?


Reply to this email directly or view it on GitHub.

@RogerRiggs

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2013

@RogerRiggs

This comment has been minimized.

Copy link
Contributor

commented Oct 2, 2013

Is the patch considered mature and ready to use or is an update needed?

@jodastephen

This comment has been minimized.

Copy link
Member Author

commented Oct 2, 2013

I haven't checked if the patch still works. However, the basic principles of the patch still look sound. Given the divergence with tl, it may be best to simply prepare a webrev vs there.

@RogerRiggs

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2013

@RogerRiggs RogerRiggs closed this Oct 15, 2013

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.