Thread: [threeten-develop] Reconsidering generics on Chrono classes
Status: Alpha
Brought to you by:
scolebourne
From: Roger R. <Rog...@or...> - 2013-02-02 21:43:23
|
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 |
From: Stephen C. <sco...@jo...> - 2013-02-02 23:26:29
|
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? 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 If there is a public XxxDate class, then (a) and (b) users are happy (with the exception of (b) user accessing ChronoLocalDateTime/ChronoZonedDateTime) 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. 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. 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. 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. Stephen On 2 February 2013 21:43, Roger Riggs <Rog...@or...> 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 > > > > > ------------------------------------------------------------------------------ > Everyone hates slow websites. So do we. > Make your web apps faster with AppDynamics > Download AppDynamics Lite for free today: > http://p.sf.net/sfu/appdyn_d2d_jan > _______________________________________________ > threeten-develop mailing list > thr...@li... > https://lists.sourceforge.net/lists/listinfo/threeten-develop |
From: Masayoshi O. <mas...@or...> - 2013-02-03 13:46:24
|
On 2/3/2013 8:26 AM, 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? > > 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 > > If there is a public XxxDate class, then (a) and (b) users are happy > (with the exception of (b) user accessing > ChronoLocalDateTime/ChronoZonedDateTime) > > 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. > > 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. > > 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. > > 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>. Sounds reasonable to me. Masayoshi > 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. > > Stephen > > > On 2 February 2013 21:43, Roger Riggs <Rog...@or...> 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 >> >> >> >> >> ------------------------------------------------------------------------------ >> Everyone hates slow websites. So do we. >> Make your web apps faster with AppDynamics >> Download AppDynamics Lite for free today: >> http://p.sf.net/sfu/appdyn_d2d_jan >> _______________________________________________ >> threeten-develop mailing list >> thr...@li... >> https://lists.sourceforge.net/lists/listinfo/threeten-develop |
From: Richard W. <ric...@gm...> - 2013-02-03 10:50:38
|
> > If there is a public XxxDate class, then (a) and (b) users are happy > (with the exception of (b) user accessing > ChronoLocalDateTime/ChronoZonedDateTime) > > 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. > In terms of use case the people who benefit from generics on calendaring systems are actually the people want to write logic in only one calendaring system, but one that isn't ISO. This does make their life easier in the sense that if they're manipulating their local date they can type their method: void doSomething(JapaneseDate theDate); instead of: void doSomething(ChronoLocalDate<JapaneseChronology> theDate); But obviously this doesn't carry over to the non-date cases, such as "ChronoLocalDateTime<JapaneseChronology>". Obviously this api isn't really targetted at those users, and we've been unable to find many people who care about this scenario anyway. Aside from thinking that Stephen raises some good points, one thing that this change might remove the possibility of is being able to add in other Chronology implementations during the JDK 8 lifecycle without needing a full on specification change. Previously this was possible by letting people lookup the Chronology using the chronology's id and relying on the fact that there was a generic api that was generic at the level of Chronology and not ChronoLocalDate. If you feel that it would still be possible even after the change then this might not be an issue. I think it has become more important since scheduling constraints have necessitated a reduction in the number of supported calendaring systems. regards, Richard Warburton http://insightfullogic.com @RichardWarburto <http://twitter.com/richardwarburto> |
From: Stephen C. <sco...@jo...> - 2013-02-03 12:21:02
|
On 3 February 2013 10:50, Richard Warburton <ric...@gm...> wrote: > Aside from thinking that Stephen raises some good points, one thing that > this change might remove the possibility of is being able to add in other > Chronology implementations during the JDK 8 lifecycle without needing a full > on specification change. > > Previously this was possible by letting people lookup the Chronology using > the chronology's id and relying on the fact that there was a generic api > that was generic at the level of Chronology and not ChronoLocalDate. If you > feel that it would still be possible even after the change then this might > not be an issue. I think it has become more important since scheduling > constraints have necessitated a reduction in the number of supported > calendaring systems. I don't think this really changes. Adding FooChronology betwen releases is only possible if it is in a hidden package like sun.time. Thus, a conforming application would have to use Chronology<?> now to refer to it. Type (b) users can't refer to ChronoLocalDate<FooChronology> (or FooDate) without refering to the hidden package. Stephen |
From: Stephen C. <sco...@jo...> - 2013-02-04 11:34:23
|
On 4 February 2013 01:09, Roger Riggs <Rog...@or...> wrote: > 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. So be it. > 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? Remove ChronoDateImpl.atTime() (pointless override of ChronoLocalDate) Make Chronology.dateTime(ChronoLocalDate<R> date, LocalTime time) package scoped. Add Javadoc (useful for those reading code rather than HTML Javadoc) * <p> * This method is called from {@link ChronoLocalDate#atTime(LocalTime)}. * Application code, and implementations of this class, should use * {@code atTime} to create a {@code ChronoLocalDateTime}. > 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. Or getEra() could be replaced by Era.from() static method. But its more useful to chrono-level (c) users as is. On 4 February 2013 04:27, Roger Riggs <Rog...@or...> wrote: > Cleaned up the methods on the Chrono and calendar types and updated the > javadoc. > > But the generics on ChronoLocalDate will need to be restored. > Without them it is not possible to write a library than can return > the same type of date passed in. Something as simple as: > > <D extends ChronoLocalDate> D next(D date) { > ! D next = date.plus(2, DAYS); > return next; > } > > fails because the type of date.plus is always ChronoLocalDate. I'm OK with that if you prefer it that way. Removing generics from Chronlogy/Era and making the date clases public is a net win I think. I reviewed the Javadoc. The main issue is that the of() factory methods on the 4 date classes are inconsistent. The full set of methods would be 1) now() 2) now(ZoneId) 3) now(Clock) 4) from(TemporalAccessor) 5) of(y,m,d) 6) of(e,y,m,d) 7) ofYearDay(y,d) 8) ofYearDay(e,y,d) 9) parse(CharSeq) 10) parse(CharSeq, DTFormatter) I think that from the perspective of (b) user, they definitely want (4) and (5) and probably (1) (2) and (3). (6), (7) and (8) seem like ones they could live without, except on JapaneseDate where (6) would be needed. The parse methods (9) and (10) would also be desirable, but could probably be lived without so long as (4) is present to use as a method reference. The implementations of these methods could delegate to the matching chronlogy for simplicity. Stephen |
From: Roger R. <Rog...@or...> - 2013-02-04 20:48:50
|
Hi, More cleanup of the factory methods and restored the generics to enable the use case below. Updated; please review: javadoc: http://cr.openjdk.java.net/~rriggs/javadoc-chrono-generics-191/ webrev: http://cr.openjdk.java.net/~rriggs/webrev-chrono-generics-191/ If this is close, I'll merge with the other pending changes and move the calendars to java.time.chrono. Then we can put it out for review and only do fine tuning until the push early next week. inline... On 2/4/2013 6:33 AM, Stephen Colebourne wrote: > On 4 February 2013 01:09, Roger Riggs<Rog...@or...> wrote: > >> 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? > Remove ChronoDateImpl.atTime() (pointless override of ChronoLocalDate) > Make Chronology.dateTime(ChronoLocalDate<R> date, LocalTime time) > package scoped. > Add Javadoc (useful for those reading code rather than HTML Javadoc) > *<p> > * This method is called from {@link ChronoLocalDate#atTime(LocalTime)}. > * Application code, and implementations of this class, should use > * {@code atTime} to create a {@code ChronoLocalDateTime}. > >> 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. Even the package private method seemed unnecessary. The implemenetation of ChronoLocalDate.atTime can call the ChronoLocalDateTimeImpl directly. Going through the chronology isn't does add any value. > Or getEra() could be replaced by Era.from() static method. But its > more useful to chrono-level (c) users as is. > > On 4 February 2013 04:27, Roger Riggs<Rog...@or...> wrote: >> Cleaned up the methods on the Chrono and calendar types and updated the >> javadoc. >> >> But the generics on ChronoLocalDate will need to be restored. >> Without them it is not possible to write a library than can return >> the same type of date passed in. Something as simple as: >> >> <D extends ChronoLocalDate> D next(D date) { >> ! D next = date.plus(2, DAYS); >> return next; >> } >> >> fails because the type of date.plus is always ChronoLocalDate. > I'm OK with that if you prefer it that way. Removing generics from > Chronlogy/Era and making the date clases public is a net win I think. yes, > > I reviewed the Javadoc. > The main issue is that the of() factory methods on the 4 date classes > are inconsistent. The full set of methods would be > 1) now() > 2) now(ZoneId) > 3) now(Clock) > 4) from(TemporalAccessor) > 5) of(y,m,d) > 6) of(e,y,m,d) > 7) ofYearDay(y,d) > 8) ofYearDay(e,y,d) > 9) parse(CharSeq) > 10) parse(CharSeq, DTFormatter) > > I think that from the perspective of (b) user, they definitely want > (4) and (5) and probably (1) (2) and (3). > > (6), (7) and (8) seem like ones they could live without, except on > JapaneseDate where (6) would be needed. ok, I implemented 1, 2,3,4, 5 and 6 for Japanese. > > The parse methods (9) and (10) would also be desirable, but could > probably be lived without so long as (4) is present to use as a method > reference. > > The implementations of these methods could delegate to the matching > chronlogy for simplicity. Yes, took the simple implementation path for now. Roger |
From: Stephen C. <sco...@jo...> - 2013-02-04 22:40:37
|
On 4 February 2013 20:48, Roger Riggs <Rog...@or...> wrote: > Updated; please review: > javadoc: > http://cr.openjdk.java.net/~rriggs/javadoc-chrono-generics-191/ > webrev: > http://cr.openjdk.java.net/~rriggs/webrev-chrono-generics-191/ > > If this is close, I'll merge with the other pending changes > and move the calendars to java.time.chrono. > > Then we can put it out for review and only do fine tuning until the > push early next week. It looks pretty good to me. Issues I saw were Javadoc based, notably the method reference example in the from(TemporalAccessor) methods. The strategy of pushing and then fixing seems OK. Stephen |
From: Roger R. <Rog...@or...> - 2013-02-04 23:19:56
|
ok, the cumulative webrevs and javadoc have been updated. javadoc: http://cr.openjdk.java.net/~rriggs/javadoc-chrono-generics-191/ webrev: http://cr.openjdk.java.net/~rriggs/webrev-chrono-generics-191/ I'll push as soon as the more complete jdk tests complete. Thanks, Roger On 2/4/2013 5:40 PM, Stephen Colebourne wrote: > On 4 February 2013 20:48, Roger Riggs<Rog...@or...> wrote: >> Updated; please review: >> javadoc: >> http://cr.openjdk.java.net/~rriggs/javadoc-chrono-generics-191/ >> webrev: >> http://cr.openjdk.java.net/~rriggs/webrev-chrono-generics-191/ >> >> If this is close, I'll merge with the other pending changes >> and move the calendars to java.time.chrono. >> >> Then we can put it out for review and only do fine tuning until the >> push early next week. > It looks pretty good to me. Issues I saw were Javadoc based, notably > the method reference example in the from(TemporalAccessor) methods. > > The strategy of pushing and then fixing seems OK. > > Stephen |
From: Stephen C. <sco...@jo...> - 2013-02-04 23:40:42
|
+1 to push Stephen On 4 February 2013 23:19, Roger Riggs <Rog...@or...> wrote: > ok, the cumulative webrevs and javadoc have been updated. > > javadoc: > http://cr.openjdk.java.net/~rriggs/javadoc-chrono-generics-191/ > webrev: > http://cr.openjdk.java.net/~rriggs/webrev-chrono-generics-191/ > > I'll push as soon as the more complete jdk tests complete. > > Thanks, Roger > > > > On 2/4/2013 5:40 PM, Stephen Colebourne wrote: > > On 4 February 2013 20:48, Roger Riggs <Rog...@or...> wrote: > > Updated; please review: > javadoc: > http://cr.openjdk.java.net/~rriggs/javadoc-chrono-generics-191/ > webrev: > http://cr.openjdk.java.net/~rriggs/webrev-chrono-generics-191/ > > If this is close, I'll merge with the other pending changes > and move the calendars to java.time.chrono. > > Then we can put it out for review and only do fine tuning until the > push early next week. > > It looks pretty good to me. Issues I saw were Javadoc based, notably > the method reference example in the from(TemporalAccessor) methods. > > The strategy of pushing and then fixing seems OK. > > Stephen > > > > > > ------------------------------------------------------------------------------ > Free Next-Gen Firewall Hardware Offer > Buy your Sophos next-gen firewall before the end March 2013 > and get the hardware for free! Learn more. > http://p.sf.net/sfu/sophos-d2d-feb > _______________________________________________ > threeten-develop mailing list > thr...@li... > https://lists.sourceforge.net/lists/listinfo/threeten-develop > |