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

[roger riggs]

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