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