mawiesne commented on code in PR #1150: URL: https://github.com/apache/opennlp/pull/1150#discussion_r3593456450
########## opennlp-core/opennlp-runtime/src/test/java/opennlp/tools/tokenize/DefaultTokenContextGeneratorTest.java: ########## @@ -0,0 +1,81 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package opennlp.tools.tokenize; + +import java.util.Arrays; +import java.util.List; + +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import opennlp.tools.util.WhitespaceMode; + +/** + * Tests for the {@link DefaultTokenContextGenerator} class. + */ +public class DefaultTokenContextGeneratorTest { + + private final DefaultTokenContextGenerator cg = new DefaultTokenContextGenerator(); + + /** + * Restores {@link WhitespaceMode} property resolution after each test, so no mode + * state leaks. + */ + @AfterEach + void resetWhitespaceMode() { + WhitespaceMode.reset(); + } + + /** + * The {@code _ws} feature class follows the active {@link WhitespaceMode}; these features + * are part of every trained tokenizer model, so this pins the default behavior at the + * code points where the modes disagree: the next line control ({@code U+0085}) is + * classified as whitespace, the information separator ({@code U+001C}) is not. + */ + @Test + void testNextLineControlProducesWhitespaceFeatureByDefault() { + WhitespaceMode.reset(); Review Comment: Why is reset() called here upfront of the actual test? Shouldn't be needed, as each `@AfterEach` call should ensure the next test is run with the exact same state, aka "reset". ########## opennlp-core/opennlp-runtime/src/test/java/opennlp/tools/tokenize/DefaultTokenContextGeneratorTest.java: ########## @@ -0,0 +1,81 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package opennlp.tools.tokenize; + +import java.util.Arrays; +import java.util.List; + +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import opennlp.tools.util.WhitespaceMode; + +/** + * Tests for the {@link DefaultTokenContextGenerator} class. + */ +public class DefaultTokenContextGeneratorTest { + + private final DefaultTokenContextGenerator cg = new DefaultTokenContextGenerator(); + + /** + * Restores {@link WhitespaceMode} property resolution after each test, so no mode + * state leaks. + */ + @AfterEach + void resetWhitespaceMode() { + WhitespaceMode.reset(); + } + + /** + * The {@code _ws} feature class follows the active {@link WhitespaceMode}; these features Review Comment: Why is there a `_` in front of `ws` in this code reference here? What does it stand for, is there a long form and/or a description of that somewhere? Please check and clarify on the meaning / semantics of that abbreviation. ########## opennlp-docs/src/docbkx/introduction.xml: ########## @@ -335,6 +335,29 @@ Arguments description: the interface 'StringInterner' and specify this class via 'opennlp.interner.class'. </para> </section> + <section xml:id="intro.sysprops.whitespace"> + <title>Whitespace Definition</title> + <para> + Tokenization, corpus format parsing, and feature generation classify whitespace + using the Unicode <code>White_Space</code> property by default. A small set of Review Comment: That is incorrect. All models trained with 1.x and 2.x code is *not* "small set", it represents _the majority!_ Please choose better wording for this doc passage and highlight the implications better. ########## opennlp-docs/src/docbkx/introduction.xml: ########## @@ -335,6 +335,29 @@ Arguments description: the interface 'StringInterner' and specify this class via 'opennlp.interner.class'. </para> </section> + <section xml:id="intro.sysprops.whitespace"> + <title>Whitespace Definition</title> + <para> + Tokenization, corpus format parsing, and feature generation classify whitespace + using the Unicode <code>White_Space</code> property by default. A small set of Review Comment: The correct form - as is in July 2026 - would be to use "all" here. As all other models are legacy in that sense! ########## opennlp-core/opennlp-runtime/src/test/java/opennlp/tools/tokenize/WhitespaceTokenizerTest.java: ########## @@ -98,4 +110,40 @@ void testTokenizationOfStringWithWindowsNewLineTokens() { Assertions.assertArrayEquals(new String[] {"a", "\r", "\n", "\r", "\n", "b", "\r", "\n", "\r", "\n", "c"}, tokenizer.tokenize("a\r\n\r\n b\r\n\r\n c")); } + + /** + * Under the default {@link WhitespaceMode#UNICODE}, the next line control ({@code U+0085}) + * separates tokens like whitespace, and the information separators ({@code U+001C..U+001F}) + * do not. + */ + @Test + void testNextLineControlIsWhitespaceByDefault() { + WhitespaceMode.reset(); Review Comment: Why is reset() called here upfront of the actual test? Shouldn't be needed, as each `@AfterEach` call should ensure the next test is run with the exact same state, aka "reset". ########## opennlp-extensions/opennlp-spellcheck/src/main/java/opennlp/spellcheck/normalizer/SpellCheckingCharSequenceNormalizer.java: ########## @@ -133,11 +135,14 @@ public SpellCheckingCharSequenceNormalizer(SpellChecker spellChecker) { * @param model the loaded model whose engine is used; must not be {@code null} */ public SpellCheckingCharSequenceNormalizer(SymSpellModel model) { - this(Objects.requireNonNull(model, "model must not be null").getSymSpell()); + this(builder(model)); } private SpellCheckingCharSequenceNormalizer(Builder b) { Review Comment: Please add Javadoc here and mention potential exceptions explicitly. ########## opennlp-core/opennlp-runtime/src/test/java/opennlp/tools/tokenize/SimpleTokenizerTest.java: ########## @@ -146,4 +158,40 @@ void testNonSpacingLetters() { Assertions.assertEquals("بنجاح", tokenizedText[3]); Assertions.assertEquals(".", tokenizedText[4]); } + + /** + * Under the default {@link WhitespaceMode#UNICODE}, the next line control ({@code U+0085}) + * separates tokens like whitespace, and the information separators ({@code U+001C..U+001F}) + * do not. + */ + @Test + void testNextLineControlIsWhitespaceByDefault() { + WhitespaceMode.reset(); Review Comment: Why is reset() called here upfront of the actual test? Shouldn't be needed, as each `@AfterEach` call should ensure the next test is run with the exact same state, aka "reset". ########## opennlp-extensions/opennlp-spellcheck/src/main/java/opennlp/spellcheck/normalizer/SpellCheckingCharSequenceNormalizer.java: ########## @@ -152,15 +157,21 @@ private SpellCheckingCharSequenceNormalizer(Builder b) { * @return a new {@link Builder} seeded with sensible defaults */ public static Builder builder(SpellChecker spellChecker) { - return new Builder(Objects.requireNonNull(spellChecker, "spellChecker must not be null")); + if (spellChecker == null) { + throw new IllegalArgumentException("spellChecker must not be null"); Review Comment: You've changed the behavior to throw IllegalArgumentException. Please change / adjust the Javadoc accordingly. ########## opennlp-extensions/opennlp-spellcheck/src/main/java/opennlp/spellcheck/normalizer/SpellCheckingCharSequenceNormalizer.java: ########## @@ -152,15 +157,21 @@ private SpellCheckingCharSequenceNormalizer(Builder b) { * @return a new {@link Builder} seeded with sensible defaults */ public static Builder builder(SpellChecker spellChecker) { - return new Builder(Objects.requireNonNull(spellChecker, "spellChecker must not be null")); + if (spellChecker == null) { + throw new IllegalArgumentException("spellChecker must not be null"); + } + return new Builder(spellChecker); } /** * @param model the loaded model whose engine to wrap; must not be {@code null} * @return a new {@link Builder} seeded with sensible defaults */ public static Builder builder(SymSpellModel model) { - return new Builder(Objects.requireNonNull(model, "model must not be null").getSymSpell()); + if (model == null) { + throw new IllegalArgumentException("model must not be null"); Review Comment: You've changed the behavior to throw IllegalArgumentException. Please change / adjust the Javadoc accordingly. ########## opennlp-extensions/opennlp-spellcheck/src/main/java/opennlp/spellcheck/normalizer/SpellCheckingCharSequenceNormalizer.java: ########## @@ -344,7 +355,10 @@ private Builder(SpellChecker spellChecker) { * @return this builder */ public Builder mode(Mode value) { - this.mode = Objects.requireNonNull(value, "mode must not be null"); + if (value == null) { + throw new IllegalArgumentException("mode must not be null"); Review Comment: You've changed the behavior to throw IllegalArgumentException. Please change / adjust the Javadoc accordingly. ########## opennlp-tools/src/test/java/opennlp/tools/util/StringUtilTest.java: ########## @@ -37,6 +47,111 @@ void testNoBreakSpace() { Assertions.assertTrue(StringUtil.isWhitespace((char) 0x202F)); } + /** + * Pins the exact semantics of {@link StringUtil#isWhitespace(int)} under the default + * {@link WhitespaceMode#UNICODE}, at the code points where the JVM predicates and the + * Unicode {@code White_Space} property disagree, so the predicate cannot drift silently: + * it excludes the {@code U+001C..U+001F} information separators and includes the next + * line control {@code U+0085}, agreeing with {@link StringUtil#isUnicodeWhitespace(int)}. + */ + @Test + void testIsWhitespaceBoundaryCodePointsDefaultIsUnicode() { + WhitespaceMode.reset(); Review Comment: Why is reset() called here upfront of the actual test? Shouldn't be needed, as each `@AfterEach` call should ensure the next test is run with the exact same state, aka "reset". -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
