<i18n dev> Request for Review : JDK-8071929 -Locale.getISOCountries() has inconsistent behaviour for "AN", "BU" and "CS" country codes.

[Rachna Goel]

Hi Naoto,

Thanks for suggestions.

updated patch is at :

http://cr.openjdk.java.net/~rgoel/JDK_8071929/webrev.04/

Thanks,
Rachna

On 06/12/16 4:47 AM, Naoto Sato wrote:
> Additional nit comments:
>
> Locale.java
>
> - Extra blank lines at line 628, 639.
>
> - The method name "getISOCountriesImpl" inside IsoCountryCode is 
> somewhat strange. How about just "createCountryCodeSet()"? Also, its 
> comment does not describe the method correctly. It does not seem a 
> "mapping function".
>
> Naoto
>
> On 12/2/16 2:35 AM, Rachna Goel wrote:
>> Hi Stephen, Masayoshi,
>>
>> Thanks for review and suggestions.
>>
>> Please have a look at updated patch on this issue.
>>
>> http://cr.openjdk.java.net/~rgoel/JDK_8071929/webrev.03/index.html
>>
>> Updations in new patch:
>>
>> 1. Updated all method names to have "ISO".
>>
>> 2. Changed implementation of mapping function to be inside enum so that
>> default case can be avoided, as suggested by Masayoshi.
>>
>> As suggested from Naoto, adding extra descriptions and clarifications
>> can be handled with a separate JBS issue.
>>
>> Thanks,
>> Rachna
>>
>>
>>
>> On 01/12/16 10:29 PM, Naoto Sato wrote:
>>> I think what we can improve here is to add extra descriptions to each
>>> enum constant. You can refer to the definitions to each country code
>>> sets at the ISO site [1]. That clarification can be done with a
>>> separate JBS issue.
>>>
>>> Naoto
>>>
>>> [1]:
>>> http://www.iso.org/iso/home/standards/country_codes/country_codes_glossary.htm 
>>>
>>>
>>> On 11/30/16 7:33 AM, Rachna Goel wrote:
>>>>  Hi Stephen,
>>>>
>>>> Please find my comments inlined.
>>>>
>>>> On 30/11/16 5:07 PM, Stephen Colebourne wrote:
>>>>> The ISO-3166-3 code is of a complex form and not widely used as 
>>>>> far as
>>>>> I know. As far as I can tell, it is not legal to create a Locale
>>>>> instance using the ISO-3166-3 code as the country.
>>>> Yes, Agreed..API doc of Locale constructors mentions that, which 
>>>> type of
>>>> country codes are valid input.
>>>>> At the very least, the PART3 constant should describe the meaning and
>>>>> structure of the codes.
>>>> I think, API doc of getISOCountries() to be updated mentions that :
>>>>
>>>>
>>>> * ISO3166-3 codes which designate country codes for those obsolete
>>>> codes,
>>>>      * can be retrieved from {@link
>>>> #getISOCountries(Locale.IsoCountryCode type)} with
>>>>       * {@code type}  {@link IsoCountryCode#PART3}.
>>>>>
>>>>> What might be useful would be a much more detailed result, where a
>>>>> user could lookup an old code and find out when it expired, and what
>>>>> replaced it.
>>>> I think that again depends on applications requirements.
>>>> For this new method may need to be added, may be handled under 
>>>> separate
>>>> JBS entry.
>>>>> Stephen
>>>>>
>>>>>
>>>>> On 30 November 2016 at 09:31, Rachna Goel <rachna.goel at oracle.com>
>>>>> wrote:
>>>>>> Hi Stephen,
>>>>>>
>>>>>> Thanks a lot for the review.
>>>>>>
>>>>>> On 30/11/16 3:15 AM, Stephen Colebourne wrote:
>>>>>>
>>>>>> I'm concerned that this is not the friendliest of new APIs. There is
>>>>>> little description of the meaning of the ISO-3166 parts - what is
>>>>>> being added is directly exposing the underlying data rather than
>>>>>> providing any kind of abstraction.
>>>>>>
>>>>>> Could you throw more light on this? Country code data is already
>>>>>> encapsulated, and there is no direct reference to map accessing 
>>>>>> those
>>>>>> data.
>>>>>> If you have some suggestions on improving it, kindly share.
>>>>>>
>>>>>> There is also an inconsistency between "ISO" and "Iso" in the
>>>>>> class/method
>>>>>> names.
>>>>>>
>>>>>> There has been lot of discussion regarding "ISO" and "Iso". CCC has
>>>>>> suggested use of "IsoCountryCode" for enum name which I proposed 
>>>>>> to be
>>>>>> "ISOCountrycode" .
>>>>>>
>>>>>> I have tried to keep constants as "ISO", variables as "iso" as per
>>>>>> with JDK
>>>>>> naming conventions. But Locale class has methods names with  "ISO",
>>>>>> So I
>>>>>> think I will update all internal method names to have "ISO".
>>>>>>
>>>>>> Thanks,
>>>>>> Rachna
>>>>>>
>>>>>>
>>>>>> Stephen
>>>>>>
>>>>>>
>>>>>> On 29 November 2016 at 09:07, Rachna Goel <rachna.goel at oracle.com>
>>>>>> wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> Please review fix for JDK-8071929.
>>>>>>
>>>>>> Bug : https://bugs.openjdk.java.net/browse/JDK-8071929
>>>>>>
>>>>>> patch : http://cr.openjdk.java.net/~rgoel/JDK_8071929/webrev.02/
>>>>>>
>>>>>> Fix is to remove obsolete country code "AN" and provide support for
>>>>>> retrieving of ISO3166-1 alpha-2,  ISO3166-1 alpha-3, ISO3166-3 
>>>>>> country
>>>>>> codes.
>>>>>>
>>>>>> Thanks,
>>>>>> Rachna
>>>>>>
>>>>>>
>>>>
>>