[threeten-dev] [threeten-develop] Initial review of changes to support Hijrah variants

[Stephen Colebourne]

- The HijrahDate factory methods should specify which variant they create.

- HijrahChronology has Javadoc "CLDR Standard extension". It isn't
obvious that is intended for use as a Locale extension.

- HijrahChronology Javadoc refers to the Gregorian year/calendar,
whereas everything else in 310 refers to the ISO year/calendar

- I think we may need to separate variants from chronologies in
getAvailableChronologies(). The Julian-Gregorian cutover calendar
(that we are not currently implementing) has a near-infinite number of
variants. Registering all the variants isn't practical. While we
aren't implementing this now, we might in the future, or we might
implement some other calendar with lots of variants.

I suggest:
getAvailableChronologies() returns the default chronology
a new insatnce (not static) method on Chronology
getAvailableVariants() returns the variants of Hijrah
Other calendar systems simply return a list of one (themselves) from
getAvailableVariants().
This would seem like a more scalable solution.

- TestHijrahChronology suggests that the getId() method of
HijrahChronology will return "Hijrah".
- The comment of HijrahChronology.getId() says it returns "Hijrah"
which isn't so.
I had assumed that HijrahChronology.INSTANCE would return getId() of
"Hijrah-umalqura". However, it would be nicer from an ID perspective
if it didn't, and only those returnes by getAvailableVariants() had
the suffix.

- The range for YEAR vs YEAR_OF_ERA looks wrong. If there are two
eras, then they cannot both have the same range. Have you actually
removed era support? Or should HijrahDate.yearofEra field be renamed?

- The checkValidDayOfYear() and similar methods can reuse code on
ValueRange, although there may be a reason not to do this (eg
performance)

- HijrahEra can only refer to a hard-coded chronology without redesign AFAICT

- There is a broader problem of the lack of support for EPOCH_MONTH in non-ISO.

- TestZoneTextPrinterParser has some commented out code. OK, but looks
like some odd code layout formatting line 169.

I haven't reviewed the accuracy of the date calculations.

Stephen


On 12 February 2013 18:38, roger riggs <roger.riggs at oracle.com> wrote:
> Hi,
>
> The changes to support multiple Hijrah calendar variants resulting
> in significant changes to the implementation of HijrahChronology,
> HijrahDate,
> and related classes.
>
> The variants are identified by new properties in lib/calendars.properties.
> Each variant is defined by a properties file containing id, type, hijrah
> start and
> corresponding Gregorian start dates plus month lengths for every month.
> The format and contents of the properties file is still being reviewed.
>
> Please review and comment:
>    http://cr.openjdk.java.net/~rriggs/webrev-hijrah-variants/
>
> javadoc:  (only the Hijrah classes are updated)
>    http://cr.openjdk.java.net/~rriggs/javadoc-hijrah-variants/
>
> The related issues are:
>   # 95 Deviation Options for the Hijrah Calendar
>   #118 Confirm calendar system type of Islamic
>
> Thanks, Roger
>
>
> ------------------------------------------------------------------------------
> 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
> threeten-develop at lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/threeten-develop
>