Re: RFR: 8054214: JapaneseEra.getDisplayName doesn't return names if it's an additional era

[Naoto Sato]

+1

Naoto

On 12/13/16 6:34 AM, Roger Riggs wrote:

Hi Masayoshi,

looks fine;  (without the javadoc)

Roger


On 12/13/2016 8:24 AM, Stephen Colebourne wrote:

The overridden method includes full Javadoc and an @since 9 tag. I
believe that this Javadoc should not be present and the @since tag is
wrong.

Stephen


On 11 December 2016 at 02:26, Masayoshi Okutsu
 wrote:

Thank you for the review comments. I realized that only
TextStyle.NARROW and
NARROW_STANDALONE should return the abbreviation to be consistent with
locale data. I've changed the implementation, which should have
"fixed" the
NPE issue, and added more test cases.

Revised webrev:
http://cr.openjdk.java.net/~okutsu/9/8054214/webrev.01/

While I was testing more, I realized the default implementation of
Era.getDisplayName doesn't work with non-IsoChronology. I filed new bug
report JDK-8171049.

Thanks,
Masayoshi


On 12/8/2016 5:55 PM, Masayoshi Okutsu wrote:

Hi,

Please review the fix for JDK-8054214. It was necessary to override
Era::getDisplayName to get era names from the specified property.
This one
fixes getAbbreviation() as well.

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

Webrev:
http://cr.openjdk.java.net/~okutsu/9/8054214/webrev.00/

Thanks,
Masayoshi

Re: RFR: 8054214: JapaneseEra.getDisplayName doesn't return names if it's an additional era

[Roger Riggs]

Hi Masayoshi,

looks fine;  (without the javadoc)

Roger


On 12/13/2016 8:24 AM, Stephen Colebourne wrote:

The overridden method includes full Javadoc and an @since 9 tag. I
believe that this Javadoc should not be present and the @since tag is
wrong.

Stephen


On 11 December 2016 at 02:26, Masayoshi Okutsu
 wrote:

Thank you for the review comments. I realized that only TextStyle.NARROW and
NARROW_STANDALONE should return the abbreviation to be consistent with
locale data. I've changed the implementation, which should have "fixed" the
NPE issue, and added more test cases.

Revised webrev:
http://cr.openjdk.java.net/~okutsu/9/8054214/webrev.01/

While I was testing more, I realized the default implementation of
Era.getDisplayName doesn't work with non-IsoChronology. I filed new bug
report JDK-8171049.

Thanks,
Masayoshi


On 12/8/2016 5:55 PM, Masayoshi Okutsu wrote:

Hi,

Please review the fix for JDK-8054214. It was necessary to override
Era::getDisplayName to get era names from the specified property. This one
fixes getAbbreviation() as well.

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

Webrev:
http://cr.openjdk.java.net/~okutsu/9/8054214/webrev.00/

Thanks,
Masayoshi

Re: RFR: 8054214: JapaneseEra.getDisplayName doesn't return names if it's an additional era

[Stephen Colebourne]

The overridden method includes full Javadoc and an @since 9 tag. I
believe that this Javadoc should not be present and the @since tag is
wrong.

Stephen


On 11 December 2016 at 02:26, Masayoshi Okutsu
 wrote:
> Thank you for the review comments. I realized that only TextStyle.NARROW and
> NARROW_STANDALONE should return the abbreviation to be consistent with
> locale data. I've changed the implementation, which should have "fixed" the
> NPE issue, and added more test cases.
>
> Revised webrev:
> http://cr.openjdk.java.net/~okutsu/9/8054214/webrev.01/
>
> While I was testing more, I realized the default implementation of
> Era.getDisplayName doesn't work with non-IsoChronology. I filed new bug
> report JDK-8171049.
>
> Thanks,
> Masayoshi
>
>
> On 12/8/2016 5:55 PM, Masayoshi Okutsu wrote:
>>
>> Hi,
>>
>> Please review the fix for JDK-8054214. It was necessary to override
>> Era::getDisplayName to get era names from the specified property. This one
>> fixes getAbbreviation() as well.
>>
>> Issue:
>> https://bugs.openjdk.java.net/browse/JDK-8054214
>>
>> Webrev:
>> http://cr.openjdk.java.net/~okutsu/9/8054214/webrev.00/
>>
>> Thanks,
>> Masayoshi
>>
>

Re: RFR: 8054214: JapaneseEra.getDisplayName doesn't return names if it's an additional era

[Masayoshi Okutsu]

Thank you for the review comments. I realized that only TextStyle.NARROW 
and NARROW_STANDALONE should return the abbreviation to be consistent 
with locale data. I've changed the implementation, which should have 
"fixed" the NPE issue, and added more test cases.


Revised webrev:
http://cr.openjdk.java.net/~okutsu/9/8054214/webrev.01/

While I was testing more, I realized the default implementation of 
Era.getDisplayName doesn't work with non-IsoChronology. I filed new bug 
report JDK-8171049.


Thanks,
Masayoshi

On 12/8/2016 5:55 PM, Masayoshi Okutsu wrote:

Hi,

Please review the fix for JDK-8054214. It was necessary to override 
Era::getDisplayName to get era names from the specified property. This 
one fixes getAbbreviation() as well.


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

Webrev:
http://cr.openjdk.java.net/~okutsu/9/8054214/webrev.00/

Thanks,
Masayoshi

Re: RFR: 8054214: JapaneseEra.getDisplayName doesn't return names if it's an additional era

[Naoto Sato]

Okutsu-san,

In JapaneseEra::getDisplayName, Should there be Objects.requireNonNull() 
check for "style"? The current impl seems to return abbreviated era if 
null is passed as "style".


Naoto

On 12/8/16 12:55 AM, Masayoshi Okutsu wrote:

Hi,

Please review the fix for JDK-8054214. It was necessary to override
Era::getDisplayName to get era names from the specified property. This
one fixes getAbbreviation() as well.

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

Webrev:
http://cr.openjdk.java.net/~okutsu/9/8054214/webrev.00/

Thanks,
Masayoshi

Re: RFR: 8054214: JapaneseEra.getDisplayName doesn't return names if it's an additional era

[Jonathan Bluett-Duncan]

Hi Masayoshi,

I'm not a reviewer, but there's something in JapaneseEra.java.cdiff.html

which
isn't terribly clear to me, so I wonder if you could clarify it for me.

In the method `getDisplayName(TextStyle style, Locale locale)`, it's not
clear to me why the call to `Objects.requireNonNull(locale, "locale");` is
within the if-statement rather than on the first line. Is this because
`Era.super::getDisplayName` already has a null check of its own?

Kind regards,
Jonathan

On 8 December 2016 at 08:55, Masayoshi Okutsu 
wrote:

> Hi,
>
> Please review the fix for JDK-8054214. It was necessary to override
> Era::getDisplayName to get era names from the specified property. This one
> fixes getAbbreviation() as well.
>
> Issue:
> https://bugs.openjdk.java.net/browse/JDK-8054214
>
> Webrev:
> http://cr.openjdk.java.net/~okutsu/9/8054214/webrev.00/
>
> Thanks,
> Masayoshi
>
>

RFR: 8054214: JapaneseEra.getDisplayName doesn't return names if it's an additional era

[Masayoshi Okutsu]

Hi,

Please review the fix for JDK-8054214. It was necessary to override 
Era::getDisplayName to get era names from the specified property. This 
one fixes getAbbreviation() as well.


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

Webrev:
http://cr.openjdk.java.net/~okutsu/9/8054214/webrev.00/

Thanks,
Masayoshi