Korean (and generic) lunar calendar implementation

1. Changed implementation of ChineseCalendar (and renamed to LunarCalendar) that it can also be used for other Asian (Korean/Japanese/Vietnamese, for example) lunar calendars.

2. Created KoreanCalendar & KoreanCalendarTest

3. Created ChineseCalendar as a subclass of LunarCalendar (it passes the original test)

Additional Note:
Since I don't know how to generate serialVersionUID for each classes, I just manually made them up for now -- so someone needs to work on it.

[zkim, 2012-03-26 19:09:58]

On 2012/03/26 18:58:40, zkim wrote:

SNV diff doesn't seem to work well because I renamed ChineseCalendar.java to
LunarCalendar.java then created another ChineseCalendar.java. You may try
downloading LunarCalendar.java and run:
$tkdiff main/classes/core/src/com/ibm/icu/util/ChineseCalendar.java
LunarCalendar.java
for better comparison.

Thanks.

[yoshito_umaoka_us.ibm.com, 2012-03-26 19:41:17]

I did not have chance to review the code yet, just because I want to wait 
for legal process. But, from your summary statement, I have some concerns 
about moving the current ChineseCalendar implementation down to the new 
ChineseCalendar (extending "new" LunarCalendar). Are you sure it does not 
break any forward serialization compatibility?

I suggested you to keep ChineseCalendar as is, and add a new subclass - 
KoreanLunarCalendar in the design mailing list because it has less 
backward/forward compatibility issues.


Also, we're probably going to deprecate ChineseDateFormat / 
ChineseDateFormatSymbols in ICU in near future. We don't want any new 
features or changes done in these classes at this point.

Another important thing you may need to look into is how to handle these 
calendars in CLDR. First of all, we need to define identifiers for new 
calendar types. Second, we need formatting date for each calendar type. In 
the Unicode CLDR working group, we recently made some structural changes 
for supporting date format in Chinese calendar. We discussed about other 
East Asian Chinese calendar variants (such as in Korea, Japan, 
Vietnam...), but we did not have chance to discuss about formatting of 
these variants yet. I also suggest you to look into this.

Thanks,
Yoshito



From:   zkim@google.com
To:     markus scherer <mscherer@google.com>, fredrik roubert 
<roubert@google.com>, jungshik shin <jungshik@google.com>, umesh nair 
<umesh@google.com>, yoshito_umaoka@us.ibm.com, 
Cc:     reply@codereview-hr.appspotmail.com
Date:   03/26/2012 03:10 PM
Subject:        Re: Korean (and generic) lunar calendar implementation 
(issue 5907071)



On 2012/03/26 18:58:40, zkim wrote:

SNV diff doesn't seem to work well because I renamed
ChineseCalendar.java to LunarCalendar.java then created another
ChineseCalendar.java. You may try downloading LunarCalendar.java and
run:
$tkdiff main/classes/core/src/com/ibm/icu/util/ChineseCalendar.java
LunarCalendar.java
for better comparison.

Thanks.

http://codereview.appspot.com/5907071/

[zkim, 2012-03-26 20:49:27]

Moved ChineseDataFormat related part to LunarCalendar class

[zkim, 2012-03-26 20:57:16]

Thanks! Please read my replies (and a couple of questions) below:

> I did not have chance to review the code yet, just because I want to wait 
> for legal process.
I am totally new here (and not a member of ICU committee). Do I need to do
anything for the legal process?

> But, from your summary statement, I have some concerns 
> about moving the current ChineseCalendar implementation down to the new 
> ChineseCalendar (extending "new" LunarCalendar). Are you sure it does not 
> break any forward serialization compatibility?

I am new to this. Can you please give me a reference to forward serialization
compatibility? Thanks!

> I suggested you to keep ChineseCalendar as is, and add a new subclass - 
> KoreanLunarCalendar in the design mailing list because it has less 
> backward/forward compatibility issues.

I had to modify the ChineseCalendar class quite a bit to make it more generic.
At most, I can combine LunarCalendar and ChineseCalendar to make it a
non-abstract class and make KoreanCalendar its subclass. However, I would like
to avoid that because that is not a good design and also the constructors are
different.

> Also, we're probably going to deprecate ChineseDateFormat / 
> ChineseDateFormatSymbols in ICU in near future. We don't want any new 
> features or changes done in these classes at this point.

Good point. I moved the ChineseDateFormat related part to the LunarCalendar
class in the 2nd patch so we can make deprecate it more easily.

> Another important thing you may need to look into is how to handle these 
> calendars in CLDR. First of all, we need to define identifiers for new 
> calendar types. Second, we need formatting date for each calendar type. In 
> the Unicode CLDR working group, we recently made some structural changes 
> for supporting date format in Chinese calendar. We discussed about other 
> East Asian Chinese calendar variants (such as in Korea, Japan, 
> Vietnam...), but we did not have chance to discuss about formatting of 
> these variants yet. I also suggest you to look into this.

I don't know much about CLDR but I don't think there are any Koreans who use
lunar calendar as a main calendar. They are only used for conversions (for lunar
new year days and birthdays) and as a supplementary note. I think it may be
similar in Japan but you should know better. ^^

[jungshik, 2012-03-27 19:48:35]

Thank you for working on this.

2012/3/26 <zkim@google.com>

> Moved ChineseDataFormat related part to LunarCalendar class
>
>
http://codereview.appspot.com/**5907071/<http://codereview.appspot.com/5907071/>
>

Sorry for kinda nitpicking. Is LunarCalendar really for pure LunarCalendar?
Note that Chinese, Korean and Vietnamese calendars are not pure lunar
calendars. They're luni-solar calendars. So, a better name for the base
class (if we want to have one) would be EastAsianLuniSolarCalendar.

Jungshik

[jungshik, 2012-03-27 19:58:27]

2012/3/26 <zkim@google.com>

> Thanks! Please read my replies (and a couple of questions) below:
>
>
>  I did not have chance to review the code yet, just because I want to
>>
> wait
>
>> for legal process.
>>
> I am totally new here (and not a member of ICU committee). Do I need to
> do anything for the legal process?
>
>
>  But, from your summary statement, I have some concerns
>> about moving the current ChineseCalendar implementation down to the
>>
> new
>
>> ChineseCalendar (extending "new" LunarCalendar). Are you sure it does
>>
> not
>
>> break any forward serialization compatibility?
>>
>
> I am new to this. Can you please give me a reference to forward
> serialization compatibility? Thanks!
>
>
>  I suggested you to keep ChineseCalendar as is, and add a new subclass
>>
> -
>
>> KoreanLunarCalendar in the design mailing list because it has less
>> backward/forward compatibility issues.
>>
>
> I had to modify the ChineseCalendar class quite a bit to make it more
> generic. At most, I can combine LunarCalendar and ChineseCalendar to
> make it a non-abstract class and make KoreanCalendar its subclass.
> However, I would like to avoid that because that is not a good design
> and also the constructors are different.
>
>
>  Also, we're probably going to deprecate ChineseDateFormat /
>> ChineseDateFormatSymbols in ICU in near future. We don't want any new
>> features or changes done in these classes at this point.
>>
>
> Good point. I moved the ChineseDateFormat related part to the
> LunarCalendar class in the 2nd patch so we can make deprecate it more
> easily.
>
>
>  Another important thing you may need to look into is how to handle
>>
> these
>
>> calendars in CLDR. First of all, we need to define identifiers for new
>>
>
>  calendar types. Second, we need formatting date for each calendar
>>
> type. In
>
>> the Unicode CLDR working group, we recently made some structural
>>
> changes
>
>> for supporting date format in Chinese calendar. We discussed about
>>
> other
>
>> East Asian Chinese calendar variants (such as in Korea, Japan,
>> Vietnam...), but we did not have chance to discuss about formatting of
>>
>
>  these variants yet. I also suggest you to look into this.
>>
>
> I don't know much about CLDR but I don't think there are any Koreans who
> use lunar calendar as a main calendar. They are only used for
> conversions (for lunar new year days and birthdays) and as a
> supplementary note. I think it may be similar in Japan but you should
> know better. ^^
>

Even though nobody uses traditional Korean calendar as their primary
calendar, I think we still need to support formatting date in that calendar
(and other East Asian traditional calendars). There are use case for
showing dates in a traditional calendar (next to date formatted in
Gregorian calendar in most cases as shown below)

2012년 1월 19일 (음 2011년 12월 26일, 甲丑)
2012년 1월 19일 (辛卯 12월 26일, 甲丑)

Once CLDR decides what formatting parameters/data are necessary for
traditional East Asian calendars, I don't see much of a problem collecting
data for that. (I'll help for one).

Jungshik



>
>
>
http://codereview.appspot.com/**5907071/<http://codereview.appspot.com/5907071/>
>

[zkim, 2012-03-27 21:26:14]

Changed LunarCalendar class' name to EastAsianLuniSolarCalendar as Jungshik
suggested

[zkim, 2012-06-15 00:19:14]

Moved to http://codereview.appspot.com/6200061/

[mscherer_g, 2012-06-28 16:24:49]

Hi, I am back from vacation. Should I update the branch in the next few
days? Is the link below the latest code to take?

On Thu, Jun 14, 2012 at 5:19 PM, <zkim@google.com> wrote:

> Moved to
http://codereview.appspot.com/**6200061/<http://codereview.appspot.com/6200061/>
>
>
http://codereview.appspot.com/**5907071/<http://codereview.appspot.com/5907071/>
>

markus

[zkim, 2012-06-28 16:34:16]

Hope you had fun vacation. ^^

Yes. The patch set #2 there is the svn diff from the original code (not
from patch set #1 -- which is your current branch, I believe). FYI, the
original code is revision #31826.
Thank you very much!

 - Zu


On Thu, Jun 28, 2012 at 9:24 AM, Markus Scherer <mscherer@google.com> wrote:

> Hi, I am back from vacation. Should I update the branch in the next few
> days? Is the link below the latest code to take?
>
> On Thu, Jun 14, 2012 at 5:19 PM, <zkim@google.com> wrote:
>
>> Moved to
http://codereview.appspot.com/**6200061/<http://codereview.appspot.com/6200061/>
>>
>>
http://codereview.appspot.com/**5907071/<http://codereview.appspot.com/5907071/>
>>
>
> markus
>