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

[roger r. <rog...@or...>]

Hi,

On 2/14/2013 2:22 PM, Stephen Colebourne wrote:
> I think Yoshito's latest mail suggests that both "sa0" and "kuwaiti"
> are not correct now for LDML.
At the moment only support ror Umm alQura is firm, as the data for the
other variants is available they can be added.  Even for this one,
the data must be acquired and validated.
>
> Chronology typo line 275  b -> by.
>
> A decision is still needed on removing BEFORE_AH
I'm going to put this off until looking at the issue about using Enum
in the Chronologies.  We may need BEFORE as a placeholder to
insure that (1) is the current era at the epoch.
>
> getId() says that it returns "Hijrah-umalqura" and "Hijrah" (@return).
> Neither statement is correct, as it could return any variant.
corrected.
>
> 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?
Not yet ready to guarantee the other variants. Creating the constants would
force their initialization which would be better off deferred.

>
> 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.
Renamed the field and parameters to use proleptic year.
>
> HijrahDate.range() does not need to override YEAR_OF_ERA is there is
> only one era.
ok,

Roger

>
> thanks
> Stephen
>
>
> On 14 February 2013 18:46, roger riggs <rog...@or...> 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 <rog...@or...> 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

-- 
Thanks, Roger

Oracle Java Platform Group

Green Oracle <http://www.oracle.com/commitment> Oracle is committed to 
developing practices and products that help protect the environment