Thanks for the comments, updates below:

Webrev:   http://cr.openjdk.java.net/~rriggs/webrev-hijrah-variants/

Javadoc:  http://cr.openjdk.java.net/~rriggs/javadoc-hijrah-variants/


On 2/19/2013 5:35 AM, Stephen Colebourne wrote:
HijrahChronology should have a blank line before
<h3>Hijrah Calendar Variant Configuration</h3>
Moreover, I assume that calendar.properties and its contents are JDK
specific, not part of the spec. So, this is an "implementation note",
not an "implementation spec"
Yes, I will change the subsection title.

I note that other variants have been removed, so we only have umalqura now.
They will be restored when we have the data to support them.

I think getCalendarType() needs to explain how the variant is handled,
as effectively that method returns two pieces of information, or add a
getCalendarVariant().
I will add a description.  CLDR only recently added the concept of a variant
and it needs to exist at the boundary where locale based tags are
mapped to Threeten semantics.  The alternative is to propagate the split
type and variant parameters into the 'of" method.

Having a single calendar type is more convenient but does raise the
issue of creating names from components and breaking apart again.
The "-" as a delimiter is ok since it is not permitted by CLDR/LDML.


public ValueRange range(ChronoField field) is incorrect wrt retaining
the BEFORE era
Please recheck, updated to include the full range of Year in the smallest
min and greatest max to allow for different calendars.
(Unless I misunderstand).

I still think that the ids "Hirah", "islamic" and "islamicc" should
map to umalqura.
CLDR proposed to deprecate "islamicc";  so that should not be included.

The chronology can be registered with multiple names; so all the lookups
return the Hijrah-umalqura chronology.

Thanks, Roger


Stephen


On 15 February 2013 21:16, roger riggs <roger.riggs@oracle.com> wrote:
Please review the changes to support multiple Islamic calendar variants:
 - Only the Umm alQura variant is supported so far and is supported by
limited
   placeholder data until the full data can be acquired and validated.
 - Tests have been updated to work with the updated API and available data
 - The use of Locale 'ca' and 'cv' values has been added to
Chronology.ofLocale
 - Properties are added to lib/calendars.properties to identify the
variants
   by calendar type and to identify the resource containing the calendar
data.

Webrev:   http://cr.openjdk.java.net/~rriggs/webrev-hijrah-variants/

Javadoc:  http://cr.openjdk.java.net/~rriggs/javadoc-hijrah-variants/

Note: there are several related issues to be addressed after this change:
 - The API for Era.date(xxx) does not lend itself to support of calendars
   with multiple variants; Eras do not have any knowledge of a specific
chronology
 - Performance of initialization at startup
 - Enumerations of Eras

--
Thanks, Roger

Oracle Java Platform Group

Oracle is committed to developing practices and products that help protect
the environment



------------------------------------------------------------------------------
The Go Parallel Website, sponsored by Intel - in partnership with Geeknet,
is your hub for all things parallel software development, from weekly
thought
leadership blogs to news, videos, case studies, tutorials, tech docs,
whitepapers, evaluation guides, and opinion stories. Check out the most
recent posts - join the conversation now.
http://goparallel.sourceforge.net/
_______________________________________________
threeten-develop mailing list
threeten-develop@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/threeten-develop