rzo1 commented on PR #1150: URL: https://github.com/apache/opennlp/pull/1150#issuecomment-4938603692
Hi @krickert - as mentioned on Slack I currently dont have the time for a manual review but I just let Fable do a comprehensive review on this PR. Here is the result: - `opennlp-api/src/main/java/opennlp/tools/tokenize/WhitespaceTokenizer.java`: Sample parsers that split training/annotation lines via `WhitespaceTokenizer.INSTANCE` (`NameSample.parse`, `POSSample`, `TokenSample`, brat/ontonotes/doccat streams) silently inherit the Unicode White_Space migration. Existing corpora containing U+001C..U+001F or U+0085 now produce different samples (and shifted `NameSample` token indices). These indirect format-parsing sites fall outside the PR's audit of direct call sites and received neither a freeze, a justification comment, nor a characterization pin, unlike the analogous direct sites (ConlluTokenSampleStream, SgmlParser, AD streams) that were frozen for exactly this reason. - `opennlp-core/opennlp-runtime/src/test/java/opennlp/tools/tokenize/WhitespaceTokenizerTest.java`: The interaction of `keepNewLines=true` with U+0085 (newly whitespace in this PR) is untested and undocumented in both `WhitespaceTokenizer` and `SimpleTokenizer`. The private `isLineSeparator` guard matches only `\n` and `\r`, so U+0085/U+2028/U+2029 now split tokens but are silently dropped instead of being emitted as newline tokens; every new test sets `setKeepNewLines(false)`, so this asymmetry is neither pinned by a test nor mentioned in the updated javadoc. Users relying on keepNewLines to reconstruct line structure get silent data loss for NEL/LS/PS input. - `opennlp-extensions/opennlp-spellcheck/src/main/java/opennlp/spellcheck/symspell/SymSpell.java`: `lookupCompound` now splits terms on Unicode White_Space but computes the final reported edit distance against `input.trim()`, which strips only chars <= U+0020. Leading/trailing NBSP/NEL runs inflate the reported distance (an NBSP-only input yields distance 1 instead of 0, unlike an ASCII-space-only input). The trim on the distance path should use the same Unicode set as the split. - `opennlp-extensions/opennlp-spellcheck/src/main/java/opennlp/spellcheck/cmdline/CorrectTextTool.java`: In `listSuggestions` the blank-line guard `line.isBlank()` still uses JVM whitespace while tokenization uses the Unicode set, so an NBSP-only line skips the newline branch and then tokenizes to zero tokens: it disappears from the output entirely, whereas an ASCII-blank line emits an empty line. Guard with the same Unicode whitespace definition or emit a newline when `tokenize()` returns empty. - `opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/sentdetect/SentenceDetectorME.java` and `opennlp-extensions/opennlp-uima/src/main/java/opennlp/uima/normalizer/NumberUtil.java`: The whitespace-semantics change is not reflected in the published javadoc of the affected API surface. `isAcceptableBreak` is protected and designed for override but documents the change only in inline comments; `NumberUtil.parse` now strips all Unicode White_Space (including NBSP/NNBSP grouping separators) but only the private helper mentions it. Both should get the same "Since 3.0 ... Unicode White_Space" note the other migrated public sites received. - `opennlp-core/opennlp-runtime/src/test/java/opennlp/tools/tokenize/SimpleTokenizerTest.java`: The private `cp(int codePoint)` helper is duplicated across eight test files touched by this PR (four within opennlp-runtime alone). Consider a shared test utility, or simply `Character.toString(int)`, which makes the helper unnecessary. Human review will follow. -- 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]
