[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-i18n-dev
Subject:    Re: <i18n dev> Request for Review : JDK-8071929 -Locale.getISOCountries() has inconsistent behaviour
From:       Naoto Sato <naoto.sato () oracle ! com>
Date:       2016-12-06 18:04:05
Message-ID: d229c758-3393-238a-1a15-be0596e4bc7c () oracle ! com
[Download RAW message or body]

Looks good to me. The map could be implemented using EnumMap with 
Collections.synchronizedMap, but I am not sure that would make any 
significant improvement in this particular case.

Naoto

On 12/5/16 10:56 PM, Rachna Goel wrote:
> 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@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@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
>>>>>>>
>>>>>>>
>>>>>
>>>
>
[prev in list] [next in list] [prev in thread] [next in thread]