<i18n dev> [11] RFR of 8146656: Wrong Months Array for DateFormatSymbols

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

<i18n dev> [11] RFR of 8146656: Wrong Months Array for DateFormatSymbols

Rachna Goel
Hi,

Kindly review API Doc fix for java.text.DateFormatSymbols.

JBS Issue : https://bugs.openjdk.java.net/browse/JDK-8146656

Webrev: http://cr.openjdk.java.net/~rgoel/8146656/webrev/

CSR: https://bugs.openjdk.java.net/browse/JDK-8191414

--
Thanks,
Rachna

Reply | Threaded
Open this post in threaded view
|

Re: <i18n dev> [11] RFR of 8146656: Wrong Months Array for DateFormatSymbols

joe darcy
Hello Rachna,


On 12/18/2017 10:35 PM, Rachna Goel wrote:

> Hi,
>
> Kindly review API Doc fix for java.text.DateFormatSymbols.
>
> JBS Issue : https://bugs.openjdk.java.net/browse/JDK-8146656
>
> Webrev: http://cr.openjdk.java.net/~rgoel/8146656/webrev/
>
> CSR: https://bugs.openjdk.java.net/browse/JDK-8191414
>

An addendum to my CSR review. The newly added text should be normative,
not just informative. That is, the text should officially be part of the
specification of the class.

If you do not want the 13 elements behavior to be required of a
subclass, change the @implNote into an @implSpec. If you want 13
elements to be required of subclasses too, replace @implNote with a
paragraph begin. (For more guidance on @impNote vs @implspec, etc. see
http://openjdk.java.net/jeps/8068562)

The CSR should be updated with the amended API change.

Thanks,

-Joe
Reply | Threaded
Open this post in threaded view
|

Re: <i18n dev> [11] RFR of 8146656: Wrong Months Array for DateFormatSymbols

Rachna Goel
Hello Joe,

Thanks for the review.

Reason I added @implNote is that it's the case for the default
implementation. Not added as a part of spec, as some implementation can
just return 12 element array for same methods through the
"java.text.spi.DateFormatSymbolsProvider" SPI.

Thanks,
Rachna

On 19/12/17 2:07 PM, joe darcy wrote:

> Hello Rachna,
>
>
> On 12/18/2017 10:35 PM, Rachna Goel wrote:
>> Hi,
>>
>> Kindly review API Doc fix for java.text.DateFormatSymbols.
>>
>> JBS Issue : https://bugs.openjdk.java.net/browse/JDK-8146656
>>
>> Webrev: http://cr.openjdk.java.net/~rgoel/8146656/webrev/
>>
>> CSR: https://bugs.openjdk.java.net/browse/JDK-8191414
>>
>
> An addendum to my CSR review. The newly added text should be
> normative, not just informative. That is, the text should officially
> be part of the specification of the class.
>
> If you do not want the 13 elements behavior to be required of a
> subclass, change the @implNote into an @implSpec. If you want 13
> elements to be required of subclasses too, replace @implNote with a
> paragraph begin. (For more guidance on @impNote vs @implspec, etc. see
> http://openjdk.java.net/jeps/8068562)
>
> The CSR should be updated with the amended API change.
>
> Thanks,
>
> -Joe

--
Thanks,
Rachna

Reply | Threaded
Open this post in threaded view
|

Re: <i18n dev> [11] RFR of 8146656: Wrong Months Array for DateFormatSymbols

joe darcy
Hi Rachna,

On 12/19/2017 1:13 AM, Rachna Goel wrote:

>
> Hello Joe,
>
> Thanks for the review.
>
> Reason I added @implNote is that it's the case for the default
> implementation. Not added as a part of spec, as some implementation
> can just return 12 element array for same methods through the
> "java.text.spi.DateFormatSymbolsProvider" SPI.
>
>

That is precisely the sort of situation the @implSpec tag is intended
for. It allows the specification to say DateFormatSymbols must behave
this way while allowing subclasses to behave differently.

Perhaps some general text can be added as normal specification,
something like

"An array with either 12 or 13 elements will be returned depending on
whether or {@link Calendar.UNDECIMBER} is supported."

paired with

@implSpec This method returns 13 elements since @link
Calendar.UNDECIMBER} is supported.

HTH,

-Joe

Reply | Threaded
Open this post in threaded view
|

Re: <i18n dev> [11] RFR of 8146656: Wrong Months Array for DateFormatSymbols

Rachna Goel
Hi Joe,

Thanks for the comments.

I have updated the CSR to have @implSpec in place of @implNote.

https://bugs.openjdk.java.net/browse/JDK-8191414

Regarding  "An array with either 12 or 13 elements will be returned
depending on whether or {@link Calendar.UNDECIMBER} is supported." ,  I
would like to go with existing statement as this method always returns
13 elements where the 13th element may be empty string or may contain
Calendar.UNDECIMBER, depending upon whether its supported by the
Calendar instance.

kindly suggest whether this looks fine!

Thanks,
Rachna


On 19/12/17 2:55 PM, joe darcy wrote:

> Hi Rachna,
>
> On 12/19/2017 1:13 AM, Rachna Goel wrote:
>>
>> Hello Joe,
>>
>> Thanks for the review.
>>
>> Reason I added @implNote is that it's the case for the default
>> implementation. Not added as a part of spec, as some implementation
>> can just return 12 element array for same methods through the
>> "java.text.spi.DateFormatSymbolsProvider" SPI.
>>
>>
>
> That is precisely the sort of situation the @implSpec tag is intended
> for. It allows the specification to say DateFormatSymbols must behave
> this way while allowing subclasses to behave differently.
>
> Perhaps some general text can be added as normal specification,
> something like
>
> "An array with either 12 or 13 elements will be returned depending on
> whether or {@link Calendar.UNDECIMBER} is supported."
>
> paired with
>
> @implSpec This method returns 13 elements since @link
> Calendar.UNDECIMBER} is supported.
>
> HTH,
>
> -Joe
>

--
Thanks,
Rachna

Reply | Threaded
Open this post in threaded view
|

Re: <i18n dev> [11] RFR of 8146656: Wrong Months Array for DateFormatSymbols

joe darcy
Hi Rachna,

I think the revised version with the @implSpec tag switch is acceptable,
but also think providing more text to describe this situation would be
helpful to programmers unaware of a 13 month possibility.

Cheers,

-Joe


On 12/19/2017 2:08 AM, Rachna Goel wrote:

>
> Hi Joe,
>
> Thanks for the comments.
>
> I have updated the CSR to have @implSpec in place of @implNote.
>
> https://bugs.openjdk.java.net/browse/JDK-8191414
>
> Regarding  "An array with either 12 or 13 elements will be returned
> depending on whether or {@link Calendar.UNDECIMBER} is supported." ,  
> I would like to go with existing statement as this method always
> returns 13 elements where the 13th element may be empty string or may
> contain Calendar.UNDECIMBER, depending upon whether its supported by
> the Calendar instance.
>
> kindly suggest whether this looks fine!
>
> Thanks,
> Rachna
>
>
> On 19/12/17 2:55 PM, joe darcy wrote:
>> Hi Rachna,
>>
>> On 12/19/2017 1:13 AM, Rachna Goel wrote:
>>>
>>> Hello Joe,
>>>
>>> Thanks for the review.
>>>
>>> Reason I added @implNote is that it's the case for the default
>>> implementation. Not added as a part of spec, as some implementation
>>> can just return 12 element array for same methods through the
>>> "java.text.spi.DateFormatSymbolsProvider" SPI.
>>>
>>>
>>
>> That is precisely the sort of situation the @implSpec tag is intended
>> for. It allows the specification to say DateFormatSymbols must behave
>> this way while allowing subclasses to behave differently.
>>
>> Perhaps some general text can be added as normal specification,
>> something like
>>
>> "An array with either 12 or 13 elements will be returned depending on
>> whether or {@link Calendar.UNDECIMBER} is supported."
>>
>> paired with
>>
>> @implSpec This method returns 13 elements since @link
>> Calendar.UNDECIMBER} is supported.
>>
>> HTH,
>>
>> -Joe
>>
>
> --
> Thanks,
> Rachna

Reply | Threaded
Open this post in threaded view
|

Re: <i18n dev> [11] RFR of 8146656: Wrong Months Array for DateFormatSymbols

Rachna Goel
Hi Joe,

I have revised this webrev to include your feedback and also updated
this CSR.

Kindly have a look at :

CSR: https://bugs.openjdk.java.net/browse/JDK-8191414

webrev: http://cr.openjdk.java.net/~rgoel/8146656/webrev.01/

Thanks,

Rachna


On 20/12/17 10:44 PM, joe darcy wrote:

>
> Hi Rachna,
>
> I think the revised version with the @implSpec tag switch is
> acceptable, but also think providing more text to describe this
> situation would be helpful to programmers unaware of a 13 month
> possibility.
>
> Cheers,
>
> -Joe
>
>
> On 12/19/2017 2:08 AM, Rachna Goel wrote:
>>
>> Hi Joe,
>>
>> Thanks for the comments.
>>
>> I have updated the CSR to have @implSpec in place of @implNote.
>>
>> https://bugs.openjdk.java.net/browse/JDK-8191414
>>
>> Regarding  "An array with either 12 or 13 elements will be returned
>> depending on whether or {@link Calendar.UNDECIMBER} is supported." ,  
>> I would like to go with existing statement as this method always
>> returns 13 elements where the 13th element may be empty string or may
>> contain Calendar.UNDECIMBER, depending upon whether its supported by
>> the Calendar instance.
>>
>> kindly suggest whether this looks fine!
>>
>> Thanks,
>> Rachna
>>
>>
>> On 19/12/17 2:55 PM, joe darcy wrote:
>>> Hi Rachna,
>>>
>>> On 12/19/2017 1:13 AM, Rachna Goel wrote:
>>>>
>>>> Hello Joe,
>>>>
>>>> Thanks for the review.
>>>>
>>>> Reason I added @implNote is that it's the case for the default
>>>> implementation. Not added as a part of spec, as some implementation
>>>> can just return 12 element array for same methods through the
>>>> "java.text.spi.DateFormatSymbolsProvider" SPI.
>>>>
>>>>
>>>
>>> That is precisely the sort of situation the @implSpec tag is
>>> intended for. It allows the specification to say DateFormatSymbols
>>> must behave this way while allowing subclasses to behave differently.
>>>
>>> Perhaps some general text can be added as normal specification,
>>> something like
>>>
>>> "An array with either 12 or 13 elements will be returned depending
>>> on whether or {@link Calendar.UNDECIMBER} is supported."
>>>
>>> paired with
>>>
>>> @implSpec This method returns 13 elements since @link
>>> Calendar.UNDECIMBER} is supported.
>>>
>>> HTH,
>>>
>>> -Joe
>>>
>>
>> --
>> Thanks,
>> Rachna
>

--
Thanks,
Rachna