Re: [15]: RFR: 8244245: localizedBy() should override localized values with default values
On 5/7/2020 9:03 AM, naoto.s...@oracle.com wrote: Hi Joe, Thank you for the review. The removed check was explicitly avoiding the default chrono/number in the locale overriding the current locale values, which is exactly what this issue is trying to remove. As Stephen wrote in another email, Unicode Extensions are correctly dealt in Chronology.ofLocale()/DecimalStyle.of() methods indirectly, so I believe no doc change is warranted. Ok, thanks for the explanation. -Joe Naoto On 5/6/20 11:32 PM, Joe Wang wrote: Hi Naoto, The Javadoc states: If the locale contains the "ca" (calendar), "nu" (numbering system), "rg" (region override), and/or "tz" (timezone) Unicode extensions, the chronology, numbering system and/or the zone are overridden. If you remove the two statements that check whether the specified locale contains "ca" or "nu", would you need to update the Javadoc as well? Best, Joe On 5/6/2020 1:44 PM, naoto.s...@oracle.com wrote: Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8244245 The CSR and proposed changeset are located at: https://bugs.openjdk.java.net/browse/JDK-8244246 https://cr.openjdk.java.net/~naoto/8244245/webrev.00/ This stems from the closed issue (https://bugs.openjdk.java.net/browse/JDK-8243162), and the rationale for this fix is discussed there. Naoto
Re: [15]: RFR: 8244245: localizedBy() should override localized values with default values
Looks good, thanks On 5/7/20 12:06 PM, naoto.s...@oracle.com wrote: Hi Roger, Thank you for the review. Wrapped the long lines as suggested, along with some typo fixes in the comments: https://cr.openjdk.java.net/~naoto/8244245/webrev.01/ Naoto On 5/7/20 7:43 AM, Roger Riggs wrote: Hi Naoto, Looks fine with a small source edit below. TestUnicodeExtension.java: Please wrap the excessively long lines; string concatination will put them together for the test. Thanks, Roger On 5/6/20 4:44 PM, naoto.s...@oracle.com wrote: Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8244245 The CSR and proposed changeset are located at: https://bugs.openjdk.java.net/browse/JDK-8244246 https://cr.openjdk.java.net/~naoto/8244245/webrev.00/ This stems from the closed issue (https://bugs.openjdk.java.net/browse/JDK-8243162), and the rationale for this fix is discussed there. Naoto
Re: [15]: RFR: 8244245: localizedBy() should override localized values with default values
On 5/7/2020 1:14 AM, Stephen Colebourne wrote: On Thu, 7 May 2020 at 07:38, Joe Wang wrote: The Javadoc states: If the locale contains the "ca" (calendar), "nu" (numbering system), "rg" (region override), and/or "tz" (timezone) Unicode extensions, the chronology, numbering system and/or the zone are overridden. If you remove the two statements that check whether the specified locale contains "ca" or "nu", would you need to update the Javadoc as well? Those things are checked by the methods Chronology.of(locale) and DecimalStyle.of(locale), so although indirect, I think the Javadoc is fine. I see. Thanks. -Joe Stephen
Re: [15]: RFR: 8244245: localizedBy() should override localized values with default values
Hi Roger, Thank you for the review. Wrapped the long lines as suggested, along with some typo fixes in the comments: https://cr.openjdk.java.net/~naoto/8244245/webrev.01/ Naoto On 5/7/20 7:43 AM, Roger Riggs wrote: Hi Naoto, Looks fine with a small source edit below. TestUnicodeExtension.java: Please wrap the excessively long lines; string concatination will put them together for the test. Thanks, Roger On 5/6/20 4:44 PM, naoto.s...@oracle.com wrote: Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8244245 The CSR and proposed changeset are located at: https://bugs.openjdk.java.net/browse/JDK-8244246 https://cr.openjdk.java.net/~naoto/8244245/webrev.00/ This stems from the closed issue (https://bugs.openjdk.java.net/browse/JDK-8243162), and the rationale for this fix is discussed there. Naoto
Re: [15]: RFR: 8244245: localizedBy() should override localized values with default values
Hi Joe, Thank you for the review. The removed check was explicitly avoiding the default chrono/number in the locale overriding the current locale values, which is exactly what this issue is trying to remove. As Stephen wrote in another email, Unicode Extensions are correctly dealt in Chronology.ofLocale()/DecimalStyle.of() methods indirectly, so I believe no doc change is warranted. Naoto On 5/6/20 11:32 PM, Joe Wang wrote: Hi Naoto, The Javadoc states: If the locale contains the "ca" (calendar), "nu" (numbering system), "rg" (region override), and/or "tz" (timezone) Unicode extensions, the chronology, numbering system and/or the zone are overridden. If you remove the two statements that check whether the specified locale contains "ca" or "nu", would you need to update the Javadoc as well? Best, Joe On 5/6/2020 1:44 PM, naoto.s...@oracle.com wrote: Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8244245 The CSR and proposed changeset are located at: https://bugs.openjdk.java.net/browse/JDK-8244246 https://cr.openjdk.java.net/~naoto/8244245/webrev.00/ This stems from the closed issue (https://bugs.openjdk.java.net/browse/JDK-8243162), and the rationale for this fix is discussed there. Naoto
Re: [15]: RFR: 8244245: localizedBy() should override localized values with default values
Hi Naoto, Looks fine with a small source edit below. TestUnicodeExtension.java: Please wrap the excessively long lines; string concatination will put them together for the test. Thanks, Roger On 5/6/20 4:44 PM, naoto.s...@oracle.com wrote: Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8244245 The CSR and proposed changeset are located at: https://bugs.openjdk.java.net/browse/JDK-8244246 https://cr.openjdk.java.net/~naoto/8244245/webrev.00/ This stems from the closed issue (https://bugs.openjdk.java.net/browse/JDK-8243162), and the rationale for this fix is discussed there. Naoto
Re: [15]: RFR: 8244245: localizedBy() should override localized values with default values
On Thu, 7 May 2020 at 07:38, Joe Wang wrote: > The Javadoc states: > If the locale contains the "ca" (calendar), "nu" (numbering > system), "rg" (region override), and/or "tz" (timezone) Unicode > extensions, the chronology, numbering system and/or the zone are overridden. > > If you remove the two statements that check whether the specified locale > contains "ca" or "nu", would you need to update the Javadoc as well? Those things are checked by the methods Chronology.of(locale) and DecimalStyle.of(locale), so although indirect, I think the Javadoc is fine. Stephen
Re: [15]: RFR: 8244245: localizedBy() should override localized values with default values
Hi Naoto, The Javadoc states: If the locale contains the "ca" (calendar), "nu" (numbering system), "rg" (region override), and/or "tz" (timezone) Unicode extensions, the chronology, numbering system and/or the zone are overridden. If you remove the two statements that check whether the specified locale contains "ca" or "nu", would you need to update the Javadoc as well? Best, Joe On 5/6/2020 1:44 PM, naoto.s...@oracle.com wrote: Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8244245 The CSR and proposed changeset are located at: https://bugs.openjdk.java.net/browse/JDK-8244246 https://cr.openjdk.java.net/~naoto/8244245/webrev.00/ This stems from the closed issue (https://bugs.openjdk.java.net/browse/JDK-8243162), and the rationale for this fix is discussed there. Naoto
Re: [15]: RFR: 8244245: localizedBy() should override localized values with default values
+1 Stephen On Wed, 6 May 2020 at 21:50, wrote: > > Hello, > > Please review the fix to the following issue: > > https://bugs.openjdk.java.net/browse/JDK-8244245 > > The CSR and proposed changeset are located at: > > https://bugs.openjdk.java.net/browse/JDK-8244246 > https://cr.openjdk.java.net/~naoto/8244245/webrev.00/ > > This stems from the closed issue > (https://bugs.openjdk.java.net/browse/JDK-8243162), and the rationale > for this fix is discussed there. > > Naoto >
[15]: RFR: 8244245: localizedBy() should override localized values with default values
Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8244245 The CSR and proposed changeset are located at: https://bugs.openjdk.java.net/browse/JDK-8244246 https://cr.openjdk.java.net/~naoto/8244245/webrev.00/ This stems from the closed issue (https://bugs.openjdk.java.net/browse/JDK-8243162), and the rationale for this fix is discussed there. Naoto
