rzo1 commented on code in PR #1150:
URL: https://github.com/apache/opennlp/pull/1150#discussion_r3568640906
##########
opennlp-api/src/main/java/opennlp/tools/tokenize/WhitespaceTokenizer.java:
##########
@@ -79,8 +83,12 @@ public Span[] tokenizePos(String d) {
return tokens.toArray(new Span[0]);
}
+ /**
+ * Checks whether {@code character} is a line separator: LF, CR, NEL, LS, or
PS.
+ */
private boolean isLineSeparator(char character) {
Review Comment:
Might be something for StringUtils, no?
##########
opennlp-api/src/main/java/opennlp/tools/tokenize/BertNormalization.java:
##########
@@ -62,6 +62,9 @@ static boolean isControl(int codePoint) {
/**
* A whitespace character in the BERT sense: space, tab, newline, carriage
* return, or Unicode space separators ({@code Zs}).
+ *
+ * <p>This definition is deliberately not the Unicode {@code White_Space}
set and stays
Review Comment:
It is unclear what "frozen" is in this context. Perhaps drop that comment.
The existing sentence is enough imho to explain the rational here..
##########
opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/sentdetect/DefaultSDContextGenerator.java:
##########
@@ -27,7 +27,10 @@
/**
* Generate event contexts for maxent decisions for sentence detection.
- *
+ * <p>
+ * The whitespace checks deliberately stay on the frozen {@link
StringUtil#isWhitespace(char)}
Review Comment:
Can be dropped,. IMHO
##########
opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/tokenize/DefaultTokenContextGenerator.java:
##########
@@ -114,6 +114,11 @@ protected List<String> createContext(String sentence, int
index) {
* a fixed text sequence depending on {@code c}. The resulting combination
is added
* to the given list {@code preds}.
*
+ * <p>
+ * The {@code _ws} class deliberately stays on the frozen
Review Comment:
unclear what "frozen" is here. Might need a rephrase to make clear, that it
relies on the 1.x and 2.x whitespace definition of OpenNLP. No suggestion for a
better phrasing though.
##########
opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/tokenize/SimpleTokenizer.java:
##########
@@ -106,8 +111,12 @@ else if (Character.isDigit(c)) {
return tokens.toArray(new Span[0]);
}
+ /**
+ * Checks whether {@code character} is a line separator: LF, CR, NEL, LS, or
PS.
+ */
private boolean isLineSeparator(char character) {
- return character == Character.LINE_SEPARATOR || character ==
Character.LETTER_NUMBER;
+ return character == '\n' || character == '\r'
Review Comment:
Might be something for StringUtils, no? (2nd duplication)
--
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]
