Re: RFR: 8282819: Deprecate Locale class constructors [v3]
On Mon, 28 Mar 2022 21:06:35 GMT, Roger Riggs wrote: >> While it is true for completeness, I would limit the addition of new method >> as little as possible, because there are already several ways to obtain a >> Locale object. > > As with other varargs method calls, it is possible to pass an array with the > values. > I think it would be useful to describe the arguments as using the varargs > nomenclature and indicate > the values are in the array. For example, the `java.util.List.of(E... > elements)` method is explicit > about the array. Anther API using varargs EnumSet. Added the explanation following the `List.of(E... elements)` javadoc. - PR: https://git.openjdk.java.net/jdk/pull/7947
Re: RFR: 8282819: Deprecate Locale class constructors [v3]
On Mon, 28 Mar 2022 16:00:14 GMT, Naoto Sato wrote:
>> src/java.base/share/classes/java/util/Locale.java line 819:
>>
>>> 817: * @since 19
>>> 818: */
>>> 819: public static Locale of(String... fields) {
>>
>> Arguably, there should be `Locale.of` overloads taking 0 to 4 arguments, so
>> that it’s not necessary to box the fields in a `String` array.
>
> While it is true for completeness, I would limit the addition of new method
> as little as possible, because there are already several ways to obtain a
> Locale object.
As with other varargs method calls, it is possible to pass an array with the
values.
I think it would be useful to describe the arguments as using the varargs
nomenclature and indicate
the values are in the array. For example, the `java.util.List.of(E...
elements)` method is explicit
about the array. Anther API using varargs EnumSet.
-
PR: https://git.openjdk.java.net/jdk/pull/7947
Re: RFR: 8282819: Deprecate Locale class constructors [v3]
On Sun, 27 Mar 2022 08:45:01 GMT, ExE Boss wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixed a build failure
>
> src/java.base/share/classes/java/util/Locale.java line 819:
>
>> 817: * @since 19
>> 818: */
>> 819: public static Locale of(String... fields) {
>
> Arguably, there should be `Locale.of` overloads taking 0 to 4 arguments, so
> that it’s not necessary to box the fields in a `String` array.
While it is true for completeness, I would limit the addition of new method as
little as possible, because there are already several ways to obtain a Locale
object.
> src/java.base/share/classes/java/util/Locale.java line 825:
>
>> 823: case 2 -> getInstance(fields[0], "", fields[1], "", null);
>> 824: case 3 -> getInstance(fields[0], "", fields[1], fields[2],
>> null);
>> 825: default -> getInstance(fields[0], fields[3], fields[1],
>> fields[2], null);
>
> This should probably throw `IllegalArgumentException` when more than 4 fields
> are passed:
> Suggestion:
>
> case 4 -> getInstance(fields[0], fields[3], fields[1], fields[2],
> null);
> default -> throw new IllegalArgumentException(/* TODO: message
> */);
Thanks for the suggestion. Will incorporate the exception in the spec/impl.
-
PR: https://git.openjdk.java.net/jdk/pull/7947
Re: RFR: 8282819: Deprecate Locale class constructors [v3]
On Sat, 26 Mar 2022 11:30:53 GMT, Lance Andersen wrote: > One suggestion As part of the PR, we should probably update > test/jdk/java/util/Locale/LocaleTest.java. or perhaps add a new test for > Locale.of() for completeness. Thanks, Lance. Sure, I will add some new unit tests for the new method. - PR: https://git.openjdk.java.net/jdk/pull/7947
Re: RFR: 8282819: Deprecate Locale class constructors [v3]
On Fri, 25 Mar 2022 22:51:23 GMT, Naoto Sato wrote:
>> Proposing to deprecate the constructors in the `java.util.Locale` class.
>> There is already a factory method and a builder to return singletons, so
>> there is no need to have constructors anymore unless one purposefully wants
>> to create `ill-formed` Locale objects, which is discouraged. We cannot
>> terminally deprecate those constructors for the compatibility to serialized
>> objects created with older JDKs. Please see the draft CSR for more detail.
>
> Naoto Sato has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fixed a build failure
src/java.base/share/classes/java/util/Locale.java line 819:
> 817: * @since 19
> 818: */
> 819: public static Locale of(String... fields) {
Arguably, there should be `Locale.of` overloads taking 0 to 4 arguments, so
that it’s not necessary to box the fields in a `String` array.
src/java.base/share/classes/java/util/Locale.java line 825:
> 823: case 2 -> getInstance(fields[0], "", fields[1], "", null);
> 824: case 3 -> getInstance(fields[0], "", fields[1], fields[2],
> null);
> 825: default -> getInstance(fields[0], fields[3], fields[1],
> fields[2], null);
This should probably throw `IllegalArgumentException` when more than 4 fields
are passed:
Suggestion:
case 4 -> getInstance(fields[0], fields[3], fields[1], fields[2],
null);
default -> throw new IllegalArgumentException(/* TODO: message */);
-
PR: https://git.openjdk.java.net/jdk/pull/7947
Re: RFR: 8282819: Deprecate Locale class constructors [v3]
On Fri, 25 Mar 2022 22:51:23 GMT, Naoto Sato wrote: >> Proposing to deprecate the constructors in the `java.util.Locale` class. >> There is already a factory method and a builder to return singletons, so >> there is no need to have constructors anymore unless one purposefully wants >> to create `ill-formed` Locale objects, which is discouraged. We cannot >> terminally deprecate those constructors for the compatibility to serialized >> objects created with older JDKs. Please see the draft CSR for more detail. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Fixed a build failure Hi Naoto, I think the changes look good. One suggestion As part of the PR, we should probably update test/jdk/java/util/Locale/LocaleTest.java. or perhaps add a new test for Locale.of() for completeness. - PR: https://git.openjdk.java.net/jdk/pull/7947
Re: RFR: 8282819: Deprecate Locale class constructors [v3]
> Proposing to deprecate the constructors in the `java.util.Locale` class. > There is already a factory method and a builder to return singletons, so > there is no need to have constructors anymore unless one purposefully wants > to create `ill-formed` Locale objects, which is discouraged. We cannot > terminally deprecate those constructors for the compatibility to serialized > objects created with older JDKs. Please see the draft CSR for more detail. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Fixed a build failure - Changes: - all: https://git.openjdk.java.net/jdk/pull/7947/files - new: https://git.openjdk.java.net/jdk/pull/7947/files/e874ff7e..b4d040af Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7947=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7947=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7947.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7947/head:pull/7947 PR: https://git.openjdk.java.net/jdk/pull/7947
