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

[Stephen Colebourne]

I think Yoshito's latest mail suggests that both "sa0" and "kuwaiti"
are not correct now for LDML.

Chronology typo line 275  b -> by.

A decision is still needed on removing BEFORE_AH

getId() says that it returns "Hijrah-umalqura" and "Hijrah" (@return).
Neither statement is correct, as it could return any variant.

It seems that there is a good case to have a public constant for each
variant, named after the variant, instead of just "INSTANCE". Or is
the intention to not guarantee other variants?

HijrahDate is still confusing, because it uses yearOfEra interally and
prolepticYear in parameters.
This code "this.yearOfEra = prolepticYear;" is logically wrong,
although the solution is probably a variable rename.

HijrahDate.range() does not need to override YEAR_OF_ERA is there is
only one era.

thanks
Stephen


On 14 February 2013 18:46, roger riggs <roger.riggs at oracle.com> wrote:
> Hi,
>
> I made several corrections and updates:
>
> Webrev:
>   http://cr.openjdk.java.net/~rriggs/webrev-hijrah-variants/
>
> Javadoc:
>    http://cr.openjdk.java.net/~rriggs/javadoc-hijrah-variants/
>
>
> On 2/14/2013 4:23 AM, Stephen Colebourne wrote:
>>
>> On 13 February 2013 20:40, roger riggs <roger.riggs at oracle.com> wrote:
>>>
>>> On 2/12/2013 6:07 PM, Stephen Colebourne wrote:
>>> - HijrahChronology has Javadoc "CLDR Standard extension". It isn't
>>> obvious that is intended for use as a Locale extension.
>>> Updated to refer to Locale extension as used by the Locale class.
>>
>> I think I'd like to see an example of how to set it using a locale
>> would be best. Thats because most developers will be unfamiliar with
>> local extensions.
>
> ok, here is an example and it raises an issue with the current
> Chronology.ofLocale
> behavior.
>
>         locale =
> Locale.forLanguageTag("en-US-u-ca-islamic-cv-hijrah-umalqura");
>         Chronology chrono = Chronology.ofLocale(locale);
>
> ;; note that Locale converts the strings to lower case.
>
> The current description only uses the "ca" value and ignores the "cv" value.
>
> The Chronology calendar type value will need to be modified to be the
> concatenation of the ca and cv values (with a '-' delimiter) so the lookups
> work.
>
>
>>> - I think we may need to separate variants from chronologies in
>>> getAvailableChronologies()
>>> That's more like a parameter than a variant.
>>> I would put the methods to modify parameters of a calendar on
>>> the chronology itself where it is needed.
>>
>> OK, lets leave it as is, with everything being registered. Its a bit
>> of a gamble, but probably a manageable one.
>>
>>> - TestHijrahChronology suggests that the getId() method of
>>> HijrahChronology will return "Hijrah".
>>>
>>> Its probably to be more explicit with Hijrah-umalqura; or something
>>> similar and readable unless translation/locale lookup will occur before
>>> presentation  formatting/parsing.
>>
>> It should return "Hijrah-umalqura" then, although parsing via
>> of(String) should map "Hijrah" to "Hijrah-umalqura".
>>
>>> 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.
>>> That would mean different Chronology instances for Hijrah and
>>> Hijrah-umalqra;
>>> but they are the same.   Can we live with the longer name (or make it
>>> easier to
>>> read with mixed case, i.e.   Hijrah-Umm-Al-Qura
>>
>> Its not the upper-casing here, "Hijrah-umalqura" is fine. But
>> of(String) must parse "Hijrah", "islamic" and "islamicc" alone.
>> Aliases.
>
> It would be unfortunate to hardcode the aliases.
>
>>> So I think the BEFORE_AH era can be removed.
>>
>> Thats your/Oracle's call. You are free to do the same for other
>> non-ISO calendars, although there is less reason to.
>>
>>> - HijrahEra can only refer to a hard-coded chronology without redesign
>>> AFAICT
>>> Or the Era.date(), Era.DateYearDay(), Ea. are removed.
>>
>> They don't add much value and can be achieved starting from the
>> chronology. OK to kill them.
>
> ok, will look at that as a separate review/commit.
>
> Roger