[
https://issues.apache.org/jira/browse/OPENNLP-1448?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17686547#comment-17686547
]
ASF GitHub Bot commented on OPENNLP-1448:
-----------------------------------------
kinow commented on code in PR #492:
URL: https://github.com/apache/opennlp/pull/492#discussion_r1101145398
##########
opennlp-distr/src/main/assembly/bin.xml:
##########
@@ -48,7 +48,17 @@
<directoryMode>755</directoryMode>
<outputDirectory>.</outputDirectory>
</fileSet>
-
+
+ <fileSet>
+ <directory>src/main/resources</directory>
+ <fileMode>644</fileMode>
+ <directoryMode>755</directoryMode>
+ <outputDirectory>conf</outputDirectory>
+ <includes>
+ <include>log4j2.xml</include>
+ </includes>
+ </fileSet>
+
Review Comment:
At least the portion of code I can see on GH UI is using spaces. But if this
file has a mix of both spaces and tabs, no need to worry about this :+1:
##########
opennlp-distr/src/main/bin/opennlp:
##########
@@ -37,4 +37,4 @@ fi
# Might fail if $0 is a link
OPENNLP_HOME=`dirname "$0"`/..
-$JAVACMD $HEAP -cp $(echo $OPENNLP_HOME/lib/*.jar | tr ' ' ':')
opennlp.tools.cmdline.CLI $@
+$JAVACMD $HEAP -Dlog4j.configurationFile=../conf/log4j2.xml -cp $(echo
$OPENNLP_HOME/lib/*.jar | tr ' ' ':') opennlp.tools.cmdline.CLI $@
Review Comment:
Add missing newline?
##########
opennlp-tools/src/main/java/opennlp/tools/log/LogPrintStream.java:
##########
@@ -0,0 +1,87 @@
+/*
+ * 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.log;
+
+import java.io.PrintStream;
+import java.util.Objects;
+
+import org.slf4j.Logger;
+import org.slf4j.event.Level;
+
+import opennlp.tools.commons.Internal;
+
+/**
+ * This class serves as an adapter for a {@link Logger} used withing a {@link
PrintStream}.
+ */
+@Internal
+public class LogPrintStream extends PrintStream {
+
+ private final Logger logger;
+ private final Level level;
+
+ /**
+ * Creates a {@link LogPrintStream} for the given {@link Logger}
+ *
+ * @param logger must not be {@code null}
+ */
+ public LogPrintStream(Logger logger) {
+ this(logger, Level.INFO);
+ }
+
+ /**
+ * Creates a {@link LogPrintStream} for the given {@link Logger}, which logs
at the specified
+ * {@link Level level}.
+ *
+ * @param logger must not be {@code null}
+ * @param level must not be {@code null}
+ */
+ public LogPrintStream(Logger logger, Level level) {
+ super(nullOutputStream());
+ Objects.requireNonNull(logger, "logger must not be NULL.");
+ Objects.requireNonNull(level, "log level must not be NULL.");
+ this.logger = logger;
+ this.level = level;
+ }
+
+ @Override
+ public PrintStream printf(String format, Object... args) {
+ log(String.format(format, args));
+ return this;
+ }
+
+ @Override
+ public void println(String msg) {
+ log(msg);
+ }
+
+ private void log(String msg) {
+ switch (level) {
+ case TRACE:
+ logger.trace(msg);
+ case DEBUG:
+ logger.debug(msg);
+ case INFO:
+ logger.info(msg);
+ case WARN:
+ logger.warn(msg);
+ case ERROR:
+ logger.error(msg);
+ }
Review Comment:
Isn't there a `logger.log(msg, level)` method in SLF4J? If so this method
could be deleted, and the caller code adapted to call that instead? (I think I
used a method like that, but can't recall the logging library, nor the
programming language :rofl: )
##########
opennlp-tools/src/main/java/opennlp/tools/ml/perceptron/SimplePerceptronSequenceTrainer.java:
##########
@@ -372,14 +362,11 @@ public void nextIteration(int iteration) throws
IOException {
if (predParams[oi] != 0) {
predParams[oi] /= totIterations;
averageParams[pi].setParameter(oi, predParams[oi]);
-
//System.err.println("updates["+pi+"]["+oi+"]=("+updates[pi][oi][ITER]+","
- // +updates[pi][oi][EVENT]+","+updates[pi][oi][VALUE]+") +
("+iterations+","+0+","
- // +params[pi].getParameters()[oi]+") ->
"+averageParams[pi].getParameters()[oi]);
}
Review Comment:
These other output lines are not useful? Maybe at another log level?
##########
opennlp-tools/src/main/java/opennlp/tools/log/LogPrintStream.java:
##########
@@ -0,0 +1,87 @@
+/*
+ * 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.log;
+
+import java.io.PrintStream;
+import java.util.Objects;
+
+import org.slf4j.Logger;
+import org.slf4j.event.Level;
+
+import opennlp.tools.commons.Internal;
+
+/**
+ * This class serves as an adapter for a {@link Logger} used withing a {@link
PrintStream}.
Review Comment:
s/withing/within
##########
opennlp-tools/src/test/java/opennlp/tools/cmdline/tokenizer/TokenizerTrainerToolTest.java:
##########
@@ -104,7 +108,10 @@ public void testTestRunHappyCase() throws IOException {
model.delete();
}
- @Test
+ //TODO OPENNLP-1447
+ @Disabled(value = "OPENNLP-1447: These kind of tests won't work anymore. " +
+ "We need to find a way to redirect log output (i.e. implement " +
+ "a custom log adapter and plug it in, if we want to do such tests.")
Review Comment:
:+1: that's common in Python, but I have never had to do that in Java. Maybe
something like:
https://stackoverflow.com/questions/29076981/how-to-intercept-slf4j-with-logback-logging-via-a-junit-test
(will comment in that issue).
> Introduce SLF4J in OpenNLP
> --------------------------
>
> Key: OPENNLP-1448
> URL: https://issues.apache.org/jira/browse/OPENNLP-1448
> Project: OpenNLP
> Issue Type: Sub-task
> Reporter: Richard Zowalla
> Assignee: Richard Zowalla
> Priority: Major
>
> This will be the first step regarding OPENNLP-1447.
> Goal is to replace System.err / System.out calls with logger output, which is
> configurable.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
