rzo1 commented on PR #1138: URL: https://github.com/apache/opennlp/pull/1138#issuecomment-4938603437
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-distr/src/main/readme/NOTICE` and `opennlp-distr/src/main/readme/LICENSE`: the binary distribution files enumerate the Unicode data files bundled in the opennlp-runtime jar (confusables.txt, WordBreakProperty.txt, ExtendedPictographic.txt) but were not updated for the newly bundled CaseFolding.txt. Root LICENSE/NOTICE and the NOTICE template were updated, so the binary distribution ships with incomplete attribution for a bundled third-party data file. - `opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/util/normalizer/TextNormalizer.java`: inconsistent contract with `TermAnalyzer.Builder`. TermAnalyzer fails loud with IllegalStateException when both `caseFold()` and `fullCaseFold()` are configured (the PR itself calls this combination always redundant), but `TextNormalizer.Builder.fullCaseFold()` silently accepts `caseFold().fullCaseFold()` and its javadoc omits the "use one or the other" warning. On top of that, `TextNormalizerTest.testEveryRungIsInvokable` now chains `.caseFold().fullCaseFold()` as the reference example, effectively blessing the anti-pattern. Either add the same guard, or mirror the javadoc warning and state that it is intentionally unenforced there, and change the test to not model the discouraged combination. - `opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/util/normalizer/FullCaseFoldCharSequenceNormalizer.java`: the class javadoc says to apply NFC composition first for decomposed input, but NFC is offset-opaque and `TextNormalizer.Builder.buildAligned()` rejects non-OffsetAwareNormalizer rungs, so the documented prerequisite cannot be satisfied in the offset-aware pipeline. `nfc().fullCaseFold()` fails at build time while `fullCaseFold()` alone silently misses folds on decomposed input. This limitation should be stated in the `buildAligned()`/`fullCaseFold()` javadoc. - `opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/util/normalizer/FullCaseFoldCharSequenceNormalizer.java`: Serializable singleton without `readResolve()`. `getInstance()` documents a shared stateless instance, but deserialization produces a second instance despite the private constructor, breaking identity-based comparisons after a round trip. Consistent with sibling normalizers, so acceptable as a follow-up hardening (ideally package-wide). - `opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/util/normalizer/TermAnalyzer.java`: the explicit `requireNotBothCaseFolds(Dimension.CASE_FOLD)` call at the top of `caseFold(Locale)` is redundant since it delegates to `transform(Dimension.CASE_FOLD, ...)`, which performs the identical check. Dropping it leaves behavior unchanged and makes the enforcement point clearer. - `opennlp-core/opennlp-runtime/src/test/java/opennlp/tools/util/normalizer/TermAnalyzerTest.java`: the mutual-exclusion guard inside `transform()` is only exercised for `CASE_FOLD`; the `FULL_CASE_FOLD` branch (`builder().caseFold().transform(Dimension.FULL_CASE_FOLD, ...)`) has no test. A one-line test would close the gap. 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]
