[threeten-dev] [threeten-develop] Reconsidering generics on Chrono classes

[Roger Riggs]

M7 is destined for the developer preview and after that even minor 
changes will
not be welcome and we're out of time.

So I'd push for the major change to be in M7.

I will cut the public APIs in the public date classes back to the public 
methods
in ChronoLocalDate* to avoid extra design work and testing since we
had agreement that ChronoLocalDate was functionally sufficient.

BTW, there are a few stray static methods in Chronology to support 
date.atTime()
and creating ChronoZonedDateTime instances.  With static methods in 
interfaces
is there a better place for them?

On 02/03/2013 07:25 PM, Stephen Colebourne wrote:
> I'm guessing that this won't be pushed tomorrow for any immediate
> freeze, as it needs a fair set of work/review. But I think it has
> potential to be cleaner for users (but since I'm not a direct user of
> this code its harder for me to tell). It certainly makes the code look
> cleaner.
>
> HijrahDate has public methods like plusDays/minusDays that you've
> overridden to return the right type. But those methods are not part of
> the current public API of ChronoLocalDate. One of the advantages of
> having a real class is that we can add these methods publicly, but we
> don't have to as part of this change.
>
> HijrahEra has "(HijrahDate)(getChronology().date(this, year, month,
> day));" which should be  "getChronology().date(this, year, month,
> day);" if the getChronology() method returns the correct type.
Since Chronology is no longer generic it cannot return the correct type.
The runtime type is correct but not the declared type.
The Era code should be using the factories in the Date classes and
won't have to go through the Chronology.  The current cast was quicker to
get compiled.
> Every date(...) factory and similar in Chronology has to be overridden
> (as in IsoChronology). Not all are yet in HijrahChronology
Yes
> Similar comments for other chronologies.
>
> While other eras are unimportant, there might be a case to make
> JapaneseEra into a public class.
I'd leave it be.
> There might also be a case for removing getEra() from ChronoLocalDate
> as it adds less value now (it could still be on JapaneseDate and
> others where it matters).
The era is still needed to be able to pass to Chronology to create dates.
> In ChronoLocalDate the atTime method returns a raw type
> ChronoLocalDateTime. This would need to be ChronoLocalDateTime<?>  as
> we can't have raw types. I don't know if ChronoLocalDateTime<FooDate>
> is a valid covariant return type override for ChronoLocalDateTime<?>,
> (and that is the key question as to whether this works).
>
> Does having double the number of public types affect the package name question?
No that has not been an issue.  If it were twice as many....

Thanks, Roger

> thanks for looking at this,
> Stephen
>
>
>
>
> On 3 February 2013 22:33, Roger Riggs<Roger.Riggs at oracle.com>  wrote:
>> Update the javadoc with the suggestion to remove generics from
>> Era, ChronoLocalDate, and Chronology.
>>
>>      http://cr.openjdk.java.net/~rriggs/javadoc-chrono-generics-191/
>> webrev:
>>      http://cr.openjdk.java.net/~rriggs/webrev-chrono-generics-191/
>>
>> Still work in progress, the samples have not been updated
>> and the javadoc may still refer to generic parameters in places.
>>
>> Roger
>>
>>
>>
>>
>>
>> On 2/3/2013 8:59 AM, Roger Riggs wrote:
>>
>> On 02/02/2013 06:26 PM, Stephen Colebourne wrote:
>>
>> I've taken a quick look. It seems that the generics have changed from
>> Chronology to date class, which strikes me as different rather than
>> better. Can you give any examples where this makes user code simpler?
>> And why you see it as an improvement?
>>
>> The move to generics around the date types makes the code written
>> easier to read without bloating the api with separate types of DT, ZDT.
>>
>> My thoughts in #191 were trying to remove generics entirely from
>> Chronology and Era. My theory is that a user will work in one of three
>> ways
>> a) only with ISO, and thus with concrete types
>> b) only with a non-ISO calendar system, and thus a public XxxDate class
>> c) only at the abstract chrono-level
>>
>> A single user with a small application might have the luxury of working in only
>> one of those modes at their own choice.  But real systems are mixes of imported
>> libraries and utilities.
>> And even a single developer may find value in developing utilities that are re-usable.
>>
>> If there is a public XxxDate class, then (a) and (b) users are happy
>> (with the exception of (b) user accessing
>> ChronoLocalDateTime/ChronoZonedDateTime)
>>
>> The generics are useful in this case to avoid a forced shift between b and c.
>>
>> The real question is whether those (c) users working with the abstract
>> chrono-level care about whether the calendar system object they are
>> passing around is the same as that of another object (which either
>> form form generics allows). The question would seem partly answered by
>> j.u.Calendar, which doesn't restrict or generify by calendar system.
>>
>> At the interface between an application wanting to write a calendar specific
>> application and a library providing calendar independent  functions the
>> generics provide type checking at the integration point.  If the library provides
>> return types parameterized by the date types, the caller does not have
>> to drop down to the abstract chrono API to handle the returned datetime values.
>>
>> We also have Oracle Globalization team comments that they would always
>> create a date in the abstract, without knowledge of the calendar
>> system. Given this, there is no way to ever populate the generic
>> argument from typical chrono-level usage. Thus whether the generic it
>> is a Chronology or a ChronoLocalDate is irrelevant.
>>
>> They also recognize the value  of being able to use both models.
>>
>> No generics on any of the five key classes (Chronology, Era,
>> ChronoLocalDate, ChronoLocalDateTime, ChronoZonedDateTime) requires
>> three public classes for each calendar system, which seems overkill.
>>
>> Yes, this is one reason to retain the generics .
>>
>> A more practical/balanced approach would be:
>> - Chronology - no generics
>> - Era - no generics
>> - ChronoLocalDate - no generics
>> - ChronoLocalDateTime<D extends ChronoLocalDate>
>> - ChronoZonedDateTime<D extends ChronoLocalDate>
>>
>> With this approach, chrono-level users (c) would use ungenerified
>> types Chronology/Era/ChronoLocalDate and wildcarded
>> ChronoLocalDateTime<?>  and ChronoZonedDateTime<?>. Whereas (b) users
>> would use XxxChronology, Era, XxxDate, ChronoLocalDateTime<XxxDate>
>> and ChronoZonedDateTime<XxxDate>.
>>
>> For me, the question here was whether this approach could be made to
>> work, as it seemed to be the approach that met the needs of users (a),
>> (b) and (c) best.
>>
>> If Chronology is not parameterized, there would be cases where it would be
>> desirable to pass in a particular known chronology and get back a date-time
>> object known to be of that chronology.
>>
>> Perhaps a minor point, but without generics on ChronoLocalDate many
>> methods would need to be overridden to return the correct type.
>>
>> I'll spend some more time this afternoon looking at this.
>>
>> Thanks, Roger
>>
>> Stephen
>>
>>
>> On 2 February 2013 21:43, Roger Riggs<Roger.Riggs at oracle.com>  wrote:
>>
>> Hi,
>>
>> I took another look at the use of generics in the Chrono classes and
>> drafted an alternative that is an improvement.
>>
>> Issue #191 mentions that the generics do not add (enough) value.
>>
>> Changing the parameter from Chronology to the concrete ChronoLocalDate type
>> is more usable for the case where the concrete date type is being used
>> deliberately
>> and enable greater fluency.
>>
>> Exposing the concrete date types along with the Chronology types makes
>> it easier
>> to write calendar specific applications and makes code using
>> ChronoLocalDateTime
>> and ChronoZonedDateTime more fluent.
>>
>> Take a look at the comment.
>> javadoc:
>>      http://cr.openjdk.java.net/~rriggs/javadoc-chrono-generics-191/
>> webrev:
>>      http://cr.openjdk.java.net/~rriggs/webrev-chrono-generics-191/
>>
>> This is not ready to commit, though the functionality of the local date
>> classes is stable, the javadoc needs improvement.
>> In particular, the exposure of useful methods in the javadoc of each class
>> is uneven since some are inherited and only appear in the inherited section
>> of the javadoc.
>>
>> Thanks, Roger
>>