On Wed, 10 Apr 2024 20:20:36 GMT, Justin Lu <j...@openjdk.org> wrote:
>> Please review this PR and associated >> [CSR](https://bugs.openjdk.org/browse/JDK-8327703) which introduces strict >> parsing for NumberFormat. >> >> The concrete subclasses that will utilize this leniency value are >> `DecimalFormat` and `CompactNumberFormat`. Strict leniency allows for >> parsing to be used as an input validation technique for localized formatted >> numbers. The default leniency mode will remain lenient, and can only be set >> to strict through an intentional `setLenient(boolean)` method call. Leniency >> operates differently based off the values returned by `isGroupingUsed()`, >> `getGroupingSize()`, and `isParseIntegerOnly()`. >> >> Below is an example of the change, the CSR can be viewed for further detail. >> >> >> DecimalFormat fmt = (DecimalFormat) >> NumberFormat.getNumberInstance(Locale.US); >> fmt.parse("1,,,0,,,00,.23Zabced45"); // returns 1000.23 >> fmt.setLenient(false); >> fmt.parse("1,,,0,,,00,.23Zabced45"); // Now throws a ParseException >> fmt.setGroupingSize(2); >> fmt.parse("12,34,567"); // throws ParseException >> fmt.setParseIntegerOnly(true) >> fmt.parse("12,34.56"); // throws ParseException >> fmt.parse("12,34"); // successfully returns the Number 1234 >> >> >> The associated tests are all localized, and the data is converted properly >> during runtime. > > Justin Lu has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains 14 additional commits since > the last revision: > > - Merge branch 'master' into JDK-8327640-NumberFormat-strictParsing > - improve wording consistency and see tags > - Merge branch 'master' into JDK-8327640-NumberFormat-strictParsing > - revert cleanup changes; (to go into a separate change) > - use directed 'inheritDoc' tag > - update example for lenient parsing > - replace protected field for private fields in subclasses for consistency > - drop Format implNote as well > - setStrict and isStrict should be optional in NumberFormat > - specify and throw UOE as default > - override and implement in dFmt and cmpctNFmt > parseStrict should be protected, and utilized by subclasses. The public > methods should just > serve as API. > - Re-work specification wording from Format down to subclasses > - ... and 4 more: https://git.openjdk.org/jdk/compare/645f53d4...aa1c284b Looks good overall. Left some minor comments. As to the tests, good to see those corner cases, but they should have unit tests for the new methods, i.e, `isStrict()` and `setStrict()`. Also I think equality/serialization for those methods should be examined. src/java.base/share/classes/java/text/DecimalFormat.java line 43: > 41: import sun.util.locale.provider.LocaleProviderAdapter; > 42: import sun.util.locale.provider.ResourceBundleBasedAdapter; > 43: Internal packages typically follow public packages. src/java.base/share/classes/java/text/DecimalFormat.java line 2653: > 2651: } > 2652: > 2653: // Checks to make sure grouping size is not violated. Used when > strict. If it is only supposed to be used in `strict`, might be helpful to put an assertion here. src/java.base/share/classes/java/text/DecimalFormat.java line 2663: > 2661: > 2662: // Calculates the index that violated the grouping size > 2663: // Calculation is determined whether it was an under or over > violation Not quite sure what `under/over violation` means here. Need more comments? src/java.base/share/classes/java/text/NumberFormat.java line 43: > 41: import sun.util.locale.provider.LocaleProviderAdapter; > 42: import sun.util.locale.provider.LocaleServiceProviderPool; > 43: Same as above src/java.base/share/classes/java/text/NumberFormat.java line 493: > 491: */ > 492: public boolean isStrict() { > 493: throw new UnsupportedOperationException(); Some message might be helpful. src/java.base/share/classes/java/text/NumberFormat.java line 512: > 510: */ > 511: public void setStrict(boolean strict) { > 512: throw new UnsupportedOperationException(); Same here ------------- PR Review: https://git.openjdk.org/jdk/pull/18339#pullrequestreview-1993112743 PR Review Comment: https://git.openjdk.org/jdk/pull/18339#discussion_r1560317735 PR Review Comment: https://git.openjdk.org/jdk/pull/18339#discussion_r1560323356 PR Review Comment: https://git.openjdk.org/jdk/pull/18339#discussion_r1560326734 PR Review Comment: https://git.openjdk.org/jdk/pull/18339#discussion_r1560330147 PR Review Comment: https://git.openjdk.org/jdk/pull/18339#discussion_r1560332122 PR Review Comment: https://git.openjdk.org/jdk/pull/18339#discussion_r1560332205
