[
https://issues.apache.org/jira/browse/OPENNLP-1975?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18121799#comment-18121799
]
ASF GitHub Bot commented on OPENNLP-1975:
-----------------------------------------
mawiesne commented on code in PR #1314:
URL: https://github.com/apache/opennlp/pull/1314#discussion_r4162806207
##########
opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/tokenize/lang/en/TokenSampleStream.java:
##########
@@ -73,7 +78,7 @@ public TokenSample next() {
default -> token;
};
if (sb.length() != 0) {
- if (!alphaNumeric.matcher(token).find() || token.startsWith("'") ||
token.equalsIgnoreCase("n't")) {
+ if (!containsLetterOrDigit(token) || token.startsWith("'") ||
token.equalsIgnoreCase("n't")) {
if ((token.equals("``") || token.equals("--") || token.equals("$") ||
Review Comment:
Extract constants here for the symbols checked. Check elsewhere if loops are
coded around such constants.
##########
opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/tokenize/lang/en/TokenSampleStream.java:
##########
@@ -113,6 +118,25 @@ public void remove() {
throw new UnsupportedOperationException();
}
+ /**
+ * Tests whether a token contains a letter or a decimal digit, by code
point. A token without
Review Comment:
Replace "Tests" with "Checks" here.
##########
opennlp-core/opennlp-formats/src/test/java/opennlp/tools/formats/conllu/ConlluStreamTest.java:
##########
@@ -119,6 +123,23 @@ void testOptionalComments() throws IOException {
}
}
+ @Test
+ void testContractionIdsAreMerged() throws IOException {
+ try (ObjectStream<ConlluSentence> stream =
getStream("es-ud-sample.conllu")) {
+ ConlluSentence sent1 = stream.read();
+
+ Assertions.assertEquals(55, sent1.getWordLines().size());
Review Comment:
Please extract `sent1.getWordLines()` into a local variable and reduce n
duplications below. Check it for null before checking by positional index.
##########
opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/util/DownloadUtil.java:
##########
@@ -255,17 +253,33 @@ private static String downloadChecksumFile(String sha512,
Path model) throws IOE
/**
* Extracts the hash from the content of a checksum file, which holds the
hash followed by the
- * name of the file it applies to.
+ * name of the file it applies to. The hash is the first run of characters
that are not Unicode
+ * {@code White_Space}, see {@link
StringUtil#splitOnUnicodeWhitespace(CharSequence)}, so the
+ * result does not depend on the whitespace mode.
+ *
+ * @param checksumFileContent The file content.
+ * @return The hash, or {@code null} if the content is {@code null} or blank.
*/
- private static String parseChecksum(String checksumFileContent) {
+ static String parseChecksum(String checksumFileContent) {
if (checksumFileContent == null) {
return null;
}
- final String trimmed = checksumFileContent.trim();
- return trimmed.isEmpty() ? null : trimmed.split("\\s")[0];
+ final String[] fields =
StringUtil.splitOnUnicodeWhitespace(checksumFileContent);
+ return fields.length == 0 ? null : fields[0];
}
+ /**
+ * Compares the SHA-512 hash of a model file with the expected one.
+ *
+ * @param model The model file.
+ * @param expectedChecksum The expected hash, or {@code null} if the
checksum file is blank.
+ * @throws IOException If the checksum file is blank, the hash cannot be
computed, or the
+ * hashes differ.
+ */
private static void verifyChecksum(Path model, String expectedChecksum)
throws IOException {
+ if (expectedChecksum == null) {
+ throw new IOException("The checksum file for " + model.getFileName() + "
is blank");
+ }
final String actualChecksum = calculateSHA512(model);
if (!actualChecksum.equalsIgnoreCase(expectedChecksum)) {
throw new IOException("SHA512 checksum validation failed for " +
model.getFileName() +
Review Comment:
IllegalStateException here. Update Javadoc.
##########
opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/util/DownloadUtil.java:
##########
@@ -255,17 +253,33 @@ private static String downloadChecksumFile(String sha512,
Path model) throws IOE
/**
* Extracts the hash from the content of a checksum file, which holds the
hash followed by the
- * name of the file it applies to.
+ * name of the file it applies to. The hash is the first run of characters
that are not Unicode
+ * {@code White_Space}, see {@link
StringUtil#splitOnUnicodeWhitespace(CharSequence)}, so the
+ * result does not depend on the whitespace mode.
+ *
+ * @param checksumFileContent The file content.
+ * @return The hash, or {@code null} if the content is {@code null} or blank.
*/
- private static String parseChecksum(String checksumFileContent) {
+ static String parseChecksum(String checksumFileContent) {
if (checksumFileContent == null) {
return null;
}
- final String trimmed = checksumFileContent.trim();
- return trimmed.isEmpty() ? null : trimmed.split("\\s")[0];
+ final String[] fields =
StringUtil.splitOnUnicodeWhitespace(checksumFileContent);
+ return fields.length == 0 ? null : fields[0];
}
+ /**
+ * Compares the SHA-512 hash of a model file with the expected one.
+ *
+ * @param model The model file.
+ * @param expectedChecksum The expected hash, or {@code null} if the
checksum file is blank.
+ * @throws IOException If the checksum file is blank, the hash cannot be
computed, or the
+ * hashes differ.
+ */
private static void verifyChecksum(Path model, String expectedChecksum)
throws IOException {
+ if (expectedChecksum == null) {
+ throw new IOException("The checksum file for " + model.getFileName() + "
is blank");
Review Comment:
That must be IllegalArgumentException, no IO... for parameter checks! Please
change and update Javadoc accordingly. Please check elsewhere.
> RegEx removal part 9: Correct the output of the regex sites in
> TokenSampleStream, ParserTool, ConlluStream, DownloadUtil and
> FeatureGeneratorUtil
> -------------------------------------------------------------------------------------------------------------------------------------------------
>
> Key: OPENNLP-1975
> URL: https://issues.apache.org/jira/browse/OPENNLP-1975
> Project: OpenNLP
> Issue Type: Bug
> Reporter: Richard Zowalla
> Assignee: Kristian Rickert
> Priority: Major
> Fix For: 3.0.0
>
> Time Spent: 1h 20m
> Remaining Estimate: 0h
>
> Split out of OPENNLP\-1928 \(PR #1275\). Depends on OPENNLP\-1928 \(RegEx
> removal part 1\).
> Replaces the regular expressions of these sites with scans that follow the
> intended format instead of copying the regex quirks. The output changes:*
> {{TokenSampleStream}} \(English\): tokens are split on Unicode White\_Space;
> a leading space no longer yields an empty first token; a line without a token
> resets the open\-quote state; a token with a letter or digit of any script is
> a word.
> * {{ParserTool}}: brackets that follow each other are each separated
> \({{a\)\)b}} gives {{a \) \) b}}\).
> * {{ConlluStream}}: a malformed multiword token id \({{3\-}}, {{\-4}},
> {{3\-\-4}}, {{1\-2\-3}}, {{4\-3}}\) fails with an {{InvalidFormatException}}
> naming the id.
> * {{DownloadUtil}}: a blank checksum file is an {{IOException}} saying so.
> * {{FeatureGeneratorUtil}}: the {{cp}} class requires exactly one capital and
> one period.
> Before merge: tokenizer eval build that reads through {{TokenSampleStream}}.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
