You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2021/01/26 11:13:43 UTC
[GitHub] [maven] McFoggy opened a new pull request #438: [MNG-7083] - introduce lazy Log message evaluation
McFoggy opened a new pull request #438:
URL: https://github.com/apache/maven/pull/438
as discussed in the dev mailing list [[1](https://mail-archives.apache.org/mod_mbox/maven-dev/202101.mbox/%3CCAGjJkv0%2BAa1ffGYnxWVYkz8_QWt7KsAYcJ0CsEe%2BMGy6h6FQ6w%40mail.gmail.com%3E)][[2](https://mail-archives.apache.org/mod_mbox/maven-dev/202101.mbox/%3CCAGjJkv2trLUX6W0UQwmsizPUNdZ6GXS9FB3ApP7w%2B7MhbBXT-g%40mail.gmail.com%3E)] this PR enhances org.apache.maven.plugin.logging.Log by adding a lazy log method with a `java.util.function.Supplier` for each log level.
For each level the supplier will be called only if the corresponding log level is active.
Using java 8 interface default method ensures backward compatibility with other potential existing implementations.
Following this checklist to help us incorporate your
contribution quickly and easily:
- [X] Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/MNG) filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
- [X] Each commit in the pull request should have a meaningful subject line and body.
- [X] Format the pull request title like `[MNG-XXX] - Fixes bug in ApproximateQuantiles`,
where you replace `MNG-XXX` with the appropriate JIRA issue. Best practice
is to use the JIRA issue title in the pull request title and in the first line of the
commit message.
- [X] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [X] Run `mvn clean verify` to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
- [X] You have run the [Core IT][core-its]
Still two unrelatd ITs tests are failing locally
````
[ERROR] Errors:
[ERROR] MavenITmng5669ReadPomsOnce>AbstractMavenIntegrationTestCase.runTest:255->testWithBuildConsumer:119 » Verification
[ERROR] MavenITmng5669ReadPomsOnce>AbstractMavenIntegrationTestCase.runTest:255->testWithoutBuildConsumer:65 » Verification
````
If your pull request is about ~20 lines of code you don't need to sign an
[Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf) if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
you have to acknowledge this by using the following check-box.
- [X] I hereby declare this contribution to be licenced under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
- [ ] In any other case, please file an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
> PS: I'll verify by asking on the mailing list if I have an ICLA already, or fill one if not.
[core-its]: https://maven.apache.org/core-its/core-it-suite/
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] rmannibucau commented on a change in pull request #438: [MNG-7083] - introduce lazy Log message evaluation
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #438:
URL: https://github.com/apache/maven/pull/438#discussion_r564557392
##########
File path: maven-plugin-api/pom.xml
##########
@@ -54,6 +54,11 @@ under the License.
<groupId>org.codehaus.plexus</groupId>
<artifactId>plexus-classworlds</artifactId>
</dependency>
+ <dependency>
Review comment:
Well for the last part mockito does way more than a custom impl and half of it is not desired I guess for such tests (mainly cause of maintenance).
On the logger impl I meant DefaultLog impl whatever is under (if it moves to something else than plexus and uses a slf4j particular binding it would be the kind of same comment).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] McFoggy commented on a change in pull request #438: [MNG-7083] - introduce lazy Log message evaluation
Posted by GitBox <gi...@apache.org>.
McFoggy commented on a change in pull request #438:
URL: https://github.com/apache/maven/pull/438#discussion_r564527024
##########
File path: maven-plugin-api/src/test/java/org/apache/maven/plugin/logging/SystemStreamLogTest.java
##########
@@ -0,0 +1,94 @@
+package org.apache.maven.plugin.logging;
+
+/*
+ * 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.
+ */
+
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+
+import java.io.PrintStream;
+import java.util.function.Supplier;
+
+/**
+ * Tests for {@link SystemStreamLog}
+ */
+public class SystemStreamLogTest
+{
+ private static final String EXPECTED_MESSAGE = "expected message";
+ private static PrintStream outStream;
+ private static PrintStream errStream;
+ private static PrintStream mockOut;
+ private static PrintStream mockErr;
+
+ /*
+ * As SystemStreamLog info/warn/error log levels are active, this test checks that
+ * a message supplier is really called/executed when logging at those levels
+ */
+ @Test
+ public void testLazyMessageIsEvaluatedForActiveLogLevels()
+ {
+ Supplier messageSupplier = Mockito.mock(Supplier.class);
+ Mockito.when(messageSupplier.get()).thenReturn(EXPECTED_MESSAGE);
+
+ Log logger = new SystemStreamLog();
+ logger.info(messageSupplier);
+ logger.warn(messageSupplier);
+ logger.error(messageSupplier);
+
+ Mockito.verify(messageSupplier, Mockito.times(3)).get();
+ }
+
+ /*
+ * As SystemStreamLog debug log level is inactive, this test checks that
+ * a message supplier is not called/executed when logging at debug level
+ */
+ @Test
+ public void testDebugLazyMessageIsNotEvaluated()
+ {
+ Supplier messageSupplier = Mockito.mock(Supplier.class);
+ Mockito.when(messageSupplier.get()).thenReturn(EXPECTED_MESSAGE);
+
+ Log logger = new SystemStreamLog();
+ logger.debug(messageSupplier);
+
+ Mockito.verify(messageSupplier, Mockito.never()).get();
+ }
+
+ @BeforeAll
+ public static void initialize()
+ {
+ outStream = System.out;
+ errStream = System.err;
+
+ mockOut = Mockito.mock(PrintStream.class);
+ System.setOut(mockOut);
+ mockErr = Mockito.mock(PrintStream.class);
Review comment:
mocks for Sytem.err & System.out removed, this part disappeared from test
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] McFoggy commented on pull request #438: [MNG-7083] - introduce lazy Log message evaluation
Posted by GitBox <gi...@apache.org>.
McFoggy commented on pull request #438:
URL: https://github.com/apache/maven/pull/438#issuecomment-767546195
> I really can't believe we are reinventing the wheel by implementing SLF4J Light.
that's not what I understood from the different discussions.
I really thought that it was expected to leverage the `org.apache.maven.plugin.logging.Log` abstraction which represents the _public_ interface that plugins should see and use.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] McFoggy commented on pull request #438: [MNG-7083] - introduce lazy Log message evaluation
Posted by GitBox <gi...@apache.org>.
McFoggy commented on pull request #438:
URL: https://github.com/apache/maven/pull/438#issuecomment-767740201
FYI ICLA received & handled by Apache secretary, signed CLAs list updated (https://people.apache.org/unlistedclas.html)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] McFoggy commented on a change in pull request #438: [MNG-7083] - introduce lazy Log message evaluation
Posted by GitBox <gi...@apache.org>.
McFoggy commented on a change in pull request #438:
URL: https://github.com/apache/maven/pull/438#discussion_r564497192
##########
File path: maven-plugin-api/src/test/java/org/apache/maven/plugin/logging/SystemStreamLogTest.java
##########
@@ -0,0 +1,94 @@
+package org.apache.maven.plugin.logging;
+
+/*
+ * 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.
+ */
+
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+
+import java.io.PrintStream;
+import java.util.function.Supplier;
+
+/**
+ * Tests for {@link SystemStreamLog}
+ */
+public class SystemStreamLogTest
+{
+ private static final String EXPECTED_MESSAGE = "expected message";
+ private static PrintStream outStream;
+ private static PrintStream errStream;
+ private static PrintStream mockOut;
+ private static PrintStream mockErr;
+
+ /*
+ * As SystemStreamLog info/warn/error log levels are active, this test checks that
+ * a message supplier is really called/executed when logging at those levels
+ */
+ @Test
+ public void testLazyMessageIsEvaluatedForActiveLogLevels()
+ {
+ Supplier messageSupplier = Mockito.mock(Supplier.class);
+ Mockito.when(messageSupplier.get()).thenReturn(EXPECTED_MESSAGE);
+
+ Log logger = new SystemStreamLog();
+ logger.info(messageSupplier);
+ logger.warn(messageSupplier);
+ logger.error(messageSupplier);
Review comment:
no because log levels are hard coded in `SystemStreamLog`. Only info, warn, error are active.
##########
File path: maven-plugin-api/src/test/java/org/apache/maven/plugin/logging/SystemStreamLogTest.java
##########
@@ -0,0 +1,94 @@
+package org.apache.maven.plugin.logging;
+
+/*
+ * 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.
+ */
+
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+
+import java.io.PrintStream;
+import java.util.function.Supplier;
+
+/**
+ * Tests for {@link SystemStreamLog}
+ */
+public class SystemStreamLogTest
+{
+ private static final String EXPECTED_MESSAGE = "expected message";
+ private static PrintStream outStream;
+ private static PrintStream errStream;
+ private static PrintStream mockOut;
+ private static PrintStream mockErr;
+
+ /*
+ * As SystemStreamLog info/warn/error log levels are active, this test checks that
+ * a message supplier is really called/executed when logging at those levels
+ */
+ @Test
+ public void testLazyMessageIsEvaluatedForActiveLogLevels()
+ {
+ Supplier messageSupplier = Mockito.mock(Supplier.class);
+ Mockito.when(messageSupplier.get()).thenReturn(EXPECTED_MESSAGE);
+
+ Log logger = new SystemStreamLog();
+ logger.info(messageSupplier);
+ logger.warn(messageSupplier);
+ logger.error(messageSupplier);
+
+ Mockito.verify(messageSupplier, Mockito.times(3)).get();
+ }
+
+ /*
+ * As SystemStreamLog debug log level is inactive, this test checks that
+ * a message supplier is not called/executed when logging at debug level
+ */
+ @Test
+ public void testDebugLazyMessageIsNotEvaluated()
+ {
+ Supplier messageSupplier = Mockito.mock(Supplier.class);
+ Mockito.when(messageSupplier.get()).thenReturn(EXPECTED_MESSAGE);
+
+ Log logger = new SystemStreamLog();
+ logger.debug(messageSupplier);
+
+ Mockito.verify(messageSupplier, Mockito.never()).get();
+ }
+
+ @BeforeAll
+ public static void initialize()
+ {
+ outStream = System.out;
+ errStream = System.err;
+
+ mockOut = Mockito.mock(PrintStream.class);
+ System.setOut(mockOut);
+ mockErr = Mockito.mock(PrintStream.class);
Review comment:
Mistake, I will change to local variable (or direct inlining).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] McFoggy commented on a change in pull request #438: [MNG-7083] - introduce lazy Log message evaluation
Posted by GitBox <gi...@apache.org>.
McFoggy commented on a change in pull request #438:
URL: https://github.com/apache/maven/pull/438#discussion_r564616540
##########
File path: maven-plugin-api/src/main/java/org/apache/maven/plugin/logging/Log.java
##########
@@ -63,6 +65,20 @@
*/
void debug( Throwable error );
+ /**
+ * Send a message to the user in the <b>debug</b> error level by computing the message
+ * only when needed. The supplier will be called only if @see #isDebugEnabled() is <b>true</b>.
+ *
+ * @param messageSupplier a non null Supplier of the message to use
+ */
+ default void debug( Supplier<String> messageSupplier )
Review comment:
default methods removed, abstract class introduced
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] McFoggy commented on a change in pull request #438: [MNG-7083] - introduce lazy Log message evaluation
Posted by GitBox <gi...@apache.org>.
McFoggy commented on a change in pull request #438:
URL: https://github.com/apache/maven/pull/438#discussion_r564526250
##########
File path: maven-plugin-api/src/test/java/org/apache/maven/plugin/logging/SystemStreamLogTest.java
##########
@@ -0,0 +1,94 @@
+package org.apache.maven.plugin.logging;
+
+/*
+ * 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.
+ */
+
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+
+import java.io.PrintStream;
+import java.util.function.Supplier;
+
+/**
+ * Tests for {@link SystemStreamLog}
+ */
+public class SystemStreamLogTest
+{
+ private static final String EXPECTED_MESSAGE = "expected message";
+ private static PrintStream outStream;
+ private static PrintStream errStream;
+ private static PrintStream mockOut;
+ private static PrintStream mockErr;
+
+ /*
+ * As SystemStreamLog info/warn/error log levels are active, this test checks that
+ * a message supplier is really called/executed when logging at those levels
+ */
+ @Test
+ public void testLazyMessageIsEvaluatedForActiveLogLevels()
+ {
+ Supplier messageSupplier = Mockito.mock(Supplier.class);
+ Mockito.when(messageSupplier.get()).thenReturn(EXPECTED_MESSAGE);
+
+ Log logger = new SystemStreamLog();
+ logger.info(messageSupplier);
+ logger.warn(messageSupplier);
+ logger.error(messageSupplier);
Review comment:
Simplification of test to be consistent with `DefaultLogTest` in latter commit
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] McFoggy commented on a change in pull request #438: [MNG-7083] - introduce lazy Log message evaluation
Posted by GitBox <gi...@apache.org>.
McFoggy commented on a change in pull request #438:
URL: https://github.com/apache/maven/pull/438#discussion_r564525577
##########
File path: maven-plugin-api/src/main/java/org/apache/maven/plugin/logging/Log.java
##########
@@ -63,6 +65,20 @@
*/
void debug( Throwable error );
+ /**
+ * Send a message to the user in the <b>debug</b> error level by computing the message
+ * only when needed. The supplier will be called only if @see #isDebugEnabled() is <b>true</b>.
+ *
+ * @param messageSupplier a non null Supplier of the message to use
+ */
+ default void debug( Supplier<String> messageSupplier )
Review comment:
new methods added in later commits
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] McFoggy commented on a change in pull request #438: [MNG-7083] - introduce lazy Log message evaluation
Posted by GitBox <gi...@apache.org>.
McFoggy commented on a change in pull request #438:
URL: https://github.com/apache/maven/pull/438#discussion_r564616279
##########
File path: maven-plugin-api/pom.xml
##########
@@ -54,6 +54,11 @@ under the License.
<groupId>org.codehaus.plexus</groupId>
<artifactId>plexus-classworlds</artifactId>
</dependency>
+ <dependency>
Review comment:
mockito removed and custom impl provided where needed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] McFoggy commented on a change in pull request #438: [MNG-7083] - introduce lazy Log message evaluation
Posted by GitBox <gi...@apache.org>.
McFoggy commented on a change in pull request #438:
URL: https://github.com/apache/maven/pull/438#discussion_r564572336
##########
File path: maven-plugin-api/pom.xml
##########
@@ -54,6 +54,11 @@ under the License.
<groupId>org.codehaus.plexus</groupId>
<artifactId>plexus-classworlds</artifactId>
</dependency>
+ <dependency>
Review comment:
I will remove mockito & change to custom/stub impl and change the tests accordingly
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] rmannibucau commented on a change in pull request #438: [MNG-7083] - introduce lazy Log message evaluation
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #438:
URL: https://github.com/apache/maven/pull/438#discussion_r564449085
##########
File path: maven-plugin-api/pom.xml
##########
@@ -54,6 +54,11 @@ under the License.
<groupId>org.codehaus.plexus</groupId>
<artifactId>plexus-classworlds</artifactId>
</dependency>
+ <dependency>
Review comment:
can we not use it and just test with a custom Log impl (like an in memory one or reusing default/logger one)? I don't see what it brings but I see how the test can pass with a broken impl ;)
##########
File path: maven-plugin-api/src/main/java/org/apache/maven/plugin/logging/Log.java
##########
@@ -63,6 +65,20 @@
*/
void debug( Throwable error );
+ /**
+ * Send a message to the user in the <b>debug</b> error level by computing the message
+ * only when needed. The supplier will be called only if @see #isDebugEnabled() is <b>true</b>.
+ *
+ * @param messageSupplier a non null Supplier of the message to use
+ */
+ default void debug( Supplier<String> messageSupplier )
Review comment:
don't think we need any default, I would add an AbstractLog our two impl would inherit from and that's it, will avoid the common default pitfalls - or corrollar is all our impl should impl it without using the default impl, up to you.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] McFoggy commented on pull request #438: [MNG-7083] - introduce lazy Log message evaluation
Posted by GitBox <gi...@apache.org>.
McFoggy commented on pull request #438:
URL: https://github.com/apache/maven/pull/438#issuecomment-767746580
If asked for, I will squash the commits.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] cstamas commented on pull request #438: [MNG-7083] - introduce lazy Log message evaluation
Posted by GitBox <gi...@apache.org>.
cstamas commented on pull request #438:
URL: https://github.com/apache/maven/pull/438#issuecomment-934774152
Proposal to supersede this https://github.com/apache/maven/pull/565
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on pull request #438: [MNG-7083] - introduce lazy Log message evaluation
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #438:
URL: https://github.com/apache/maven/pull/438#issuecomment-767524135
I really can't believe we are reinventing the wheel by implementing SLF4J Light.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] cstamas edited a comment on pull request #438: [MNG-7083] - introduce lazy Log message evaluation
Posted by GitBox <gi...@apache.org>.
cstamas edited a comment on pull request #438:
URL: https://github.com/apache/maven/pull/438#issuecomment-932717398
I really don't think we want to invent new logging API. Let me explain:
* I do agree with @rmannibucau et al that "slf4j-api should not leak into plugins", but IMHO none of the reasons listed above is the **real issue**.
* IMHO, the **real issue** is that slf4j-api is "like Guava", is literally everywhere, and (in short term future) will be _really easy to end up **(in complex plugins, kinda edge case)** with a class-path that contains 1.5, 1.7 and 2.x versions of slf4j-api (and let's assume they will be incompatible)_.
* This is true for **complex plugins** that usually drag in components from different dependencies/projects. OTOH, **simple plugins** (dependency wise, not functionality or logic wise) that has only components "rolled by it's own" CAN control which ~~slf4j-api~~ Logging API it expects and uses.
* Hence, IMHO Maven should not be an impediment (or trouble source) to developers of "complex plugins", by adding a "yet another" slf4j-api version to their (possibly already complex) equation to solve.
Still, as plugin realms are "self first", I think plugin developers are able to solve Mojo (and their component, rolled by own or imported from foreign projects a dependencies) using any logging API they want.
Considering all this above, I'd rather expect to make Mojo Logger "closer" to slf4j-api, not only to simplify our work (to adapt slf4j logger to Mojo Logger), but also as sl4j logger _is lazy as well_, without any extra code. As in case of Mojo Logger, our wrapper will protect users of Mojo logger from sl4j-api incompatibilities (is provided at runtime by Maven Core).
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] McFoggy commented on a change in pull request #438: [MNG-7083] - introduce lazy Log message evaluation
Posted by GitBox <gi...@apache.org>.
McFoggy commented on a change in pull request #438:
URL: https://github.com/apache/maven/pull/438#discussion_r564527199
##########
File path: maven-plugin-api/src/test/java/org/apache/maven/plugin/logging/SystemStreamLogTest.java
##########
@@ -0,0 +1,94 @@
+package org.apache.maven.plugin.logging;
+
+/*
+ * 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.
+ */
+
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+
+import java.io.PrintStream;
+import java.util.function.Supplier;
+
+/**
+ * Tests for {@link SystemStreamLog}
+ */
+public class SystemStreamLogTest
+{
+ private static final String EXPECTED_MESSAGE = "expected message";
+ private static PrintStream outStream;
+ private static PrintStream errStream;
+ private static PrintStream mockOut;
+ private static PrintStream mockErr;
+
+ /*
+ * As SystemStreamLog info/warn/error log levels are active, this test checks that
+ * a message supplier is really called/executed when logging at those levels
+ */
+ @Test
+ public void testLazyMessageIsEvaluatedForActiveLogLevels()
+ {
+ Supplier messageSupplier = Mockito.mock(Supplier.class);
+ Mockito.when(messageSupplier.get()).thenReturn(EXPECTED_MESSAGE);
+
+ Log logger = new SystemStreamLog();
+ logger.info(messageSupplier);
+ logger.warn(messageSupplier);
+ logger.error(messageSupplier);
+
+ Mockito.verify(messageSupplier, Mockito.times(3)).get();
+ }
+
+ /*
+ * As SystemStreamLog debug log level is inactive, this test checks that
+ * a message supplier is not called/executed when logging at debug level
+ */
+ @Test
+ public void testDebugLazyMessageIsNotEvaluated()
+ {
+ Supplier messageSupplier = Mockito.mock(Supplier.class);
+ Mockito.when(messageSupplier.get()).thenReturn(EXPECTED_MESSAGE);
+
+ Log logger = new SystemStreamLog();
+ logger.debug(messageSupplier);
Review comment:
Simplification of test to be consistent with `DefaultLogTest` in latter commit
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] McFoggy commented on a change in pull request #438: [MNG-7083] - introduce lazy Log message evaluation
Posted by GitBox <gi...@apache.org>.
McFoggy commented on a change in pull request #438:
URL: https://github.com/apache/maven/pull/438#discussion_r564502239
##########
File path: maven-plugin-api/src/main/java/org/apache/maven/plugin/logging/Log.java
##########
@@ -63,6 +65,20 @@
*/
void debug( Throwable error );
+ /**
+ * Send a message to the user in the <b>debug</b> error level by computing the message
+ * only when needed. The supplier will be called only if @see #isDebugEnabled() is <b>true</b>.
+ *
+ * @param messageSupplier a non null Supplier of the message to use
+ */
+ default void debug( Supplier<String> messageSupplier )
Review comment:
I used the default methods not to break any other potential implementation.
If it is preferred not to do so then I can go with a breaking change and introduction of an Abstract class between the implementations.
Let's wait a bit for others POV.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] cstamas commented on pull request #438: [MNG-7083] - introduce lazy Log message evaluation
Posted by GitBox <gi...@apache.org>.
cstamas commented on pull request #438:
URL: https://github.com/apache/maven/pull/438#issuecomment-932717398
I really don't think we want to invent new logging API. Let me explain:
* I do agree with @rmannibucau et al that "slf4j-api should not leak into plugins", but IMHO none of the reasons listed above is the **real issue**.
* IMHO, the **real issue** is that slf4j-api is "like Guava", is literally everywhere, and (in short term future) will be _really easy to end up **(in complex plugins, kinda edge case)** with a class-path that contains 1.5, 1.7 and 2.x versions of slf4j-api (and let's assume they will be incompatible)_.
* This is true for **complex plugins** that usually drag in components from different dependencies/projects. OTOH, **simple plugins** (dependency wise, not functionality or logic wise) that has only components "rolled by it's own" CAN control which slf4j-api it expects and uses.
* Hence, IMHO Maven should not be an impediment (or trouble) to developers of "complex plugins", by adding a "yet another" slf4j-api version to their (possibly already complex) equation to solve.
Still, as plugin realms are "self first", I think plugin developers are able to solve Mojo (and their component, rolled by own or imported from foreign projects a dependencies) using any logging API they want.
Considering all this above, I'd rather expect to make Mojo Logger "closer" to slf4j-api, not only to simplify our work (to adapt slf4j logger to Mojo Logger), but also as sl4j logger _is lazy as well_, without any extra code. As in case of Mojo Logger, our wrapper will protect users of Mojo logger from sl4j-api incompatibilities (is provided at runtime by Maven Core).
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] slachiewicz commented on pull request #438: [MNG-7083] - introduce lazy Log message evaluation
Posted by GitBox <gi...@apache.org>.
slachiewicz commented on pull request #438:
URL: https://github.com/apache/maven/pull/438#issuecomment-851013325
I'd like to see how much we can improve our core code with these changes
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] slawekjaranowski commented on a change in pull request #438: [MNG-7083] - introduce lazy Log message evaluation
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on a change in pull request #438:
URL: https://github.com/apache/maven/pull/438#discussion_r564440925
##########
File path: maven-plugin-api/src/main/java/org/apache/maven/plugin/logging/Log.java
##########
@@ -63,6 +65,20 @@
*/
void debug( Throwable error );
+ /**
+ * Send a message to the user in the <b>debug</b> error level by computing the message
+ * only when needed. The supplier will be called only if @see #isDebugEnabled() is <b>true</b>.
+ *
+ * @param messageSupplier a non null Supplier of the message to use
+ */
+ default void debug( Supplier<String> messageSupplier )
Review comment:
why do you not add methods, like
void debug( Supplier<String> messageSupplier, Throwable error )
##########
File path: maven-plugin-api/src/test/java/org/apache/maven/plugin/logging/SystemStreamLogTest.java
##########
@@ -0,0 +1,94 @@
+package org.apache.maven.plugin.logging;
+
+/*
+ * 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.
+ */
+
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+
+import java.io.PrintStream;
+import java.util.function.Supplier;
+
+/**
+ * Tests for {@link SystemStreamLog}
+ */
+public class SystemStreamLogTest
+{
+ private static final String EXPECTED_MESSAGE = "expected message";
+ private static PrintStream outStream;
+ private static PrintStream errStream;
+ private static PrintStream mockOut;
+ private static PrintStream mockErr;
+
+ /*
+ * As SystemStreamLog info/warn/error log levels are active, this test checks that
+ * a message supplier is really called/executed when logging at those levels
+ */
+ @Test
+ public void testLazyMessageIsEvaluatedForActiveLogLevels()
+ {
+ Supplier messageSupplier = Mockito.mock(Supplier.class);
+ Mockito.when(messageSupplier.get()).thenReturn(EXPECTED_MESSAGE);
+
+ Log logger = new SystemStreamLog();
+ logger.info(messageSupplier);
+ logger.warn(messageSupplier);
+ logger.error(messageSupplier);
Review comment:
missing? `logger.debug`
##########
File path: maven-plugin-api/src/test/java/org/apache/maven/plugin/logging/SystemStreamLogTest.java
##########
@@ -0,0 +1,94 @@
+package org.apache.maven.plugin.logging;
+
+/*
+ * 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.
+ */
+
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+
+import java.io.PrintStream;
+import java.util.function.Supplier;
+
+/**
+ * Tests for {@link SystemStreamLog}
+ */
+public class SystemStreamLogTest
+{
+ private static final String EXPECTED_MESSAGE = "expected message";
+ private static PrintStream outStream;
+ private static PrintStream errStream;
+ private static PrintStream mockOut;
+ private static PrintStream mockErr;
+
+ /*
+ * As SystemStreamLog info/warn/error log levels are active, this test checks that
+ * a message supplier is really called/executed when logging at those levels
+ */
+ @Test
+ public void testLazyMessageIsEvaluatedForActiveLogLevels()
+ {
+ Supplier messageSupplier = Mockito.mock(Supplier.class);
+ Mockito.when(messageSupplier.get()).thenReturn(EXPECTED_MESSAGE);
+
+ Log logger = new SystemStreamLog();
+ logger.info(messageSupplier);
+ logger.warn(messageSupplier);
+ logger.error(messageSupplier);
+
+ Mockito.verify(messageSupplier, Mockito.times(3)).get();
+ }
+
+ /*
+ * As SystemStreamLog debug log level is inactive, this test checks that
+ * a message supplier is not called/executed when logging at debug level
+ */
+ @Test
+ public void testDebugLazyMessageIsNotEvaluated()
+ {
+ Supplier messageSupplier = Mockito.mock(Supplier.class);
+ Mockito.when(messageSupplier.get()).thenReturn(EXPECTED_MESSAGE);
+
+ Log logger = new SystemStreamLog();
+ logger.debug(messageSupplier);
+
+ Mockito.verify(messageSupplier, Mockito.never()).get();
+ }
+
+ @BeforeAll
+ public static void initialize()
+ {
+ outStream = System.out;
+ errStream = System.err;
+
+ mockOut = Mockito.mock(PrintStream.class);
+ System.setOut(mockOut);
+ mockErr = Mockito.mock(PrintStream.class);
Review comment:
`mockOut`, `mockErr` - are not used in test - why do you remember reference in fields
##########
File path: maven-plugin-api/src/test/java/org/apache/maven/plugin/logging/SystemStreamLogTest.java
##########
@@ -0,0 +1,94 @@
+package org.apache.maven.plugin.logging;
+
+/*
+ * 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.
+ */
+
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+
+import java.io.PrintStream;
+import java.util.function.Supplier;
+
+/**
+ * Tests for {@link SystemStreamLog}
+ */
+public class SystemStreamLogTest
+{
+ private static final String EXPECTED_MESSAGE = "expected message";
+ private static PrintStream outStream;
+ private static PrintStream errStream;
+ private static PrintStream mockOut;
+ private static PrintStream mockErr;
+
+ /*
+ * As SystemStreamLog info/warn/error log levels are active, this test checks that
+ * a message supplier is really called/executed when logging at those levels
+ */
+ @Test
+ public void testLazyMessageIsEvaluatedForActiveLogLevels()
+ {
+ Supplier messageSupplier = Mockito.mock(Supplier.class);
+ Mockito.when(messageSupplier.get()).thenReturn(EXPECTED_MESSAGE);
+
+ Log logger = new SystemStreamLog();
+ logger.info(messageSupplier);
+ logger.warn(messageSupplier);
+ logger.error(messageSupplier);
+
+ Mockito.verify(messageSupplier, Mockito.times(3)).get();
+ }
+
+ /*
+ * As SystemStreamLog debug log level is inactive, this test checks that
+ * a message supplier is not called/executed when logging at debug level
+ */
+ @Test
+ public void testDebugLazyMessageIsNotEvaluated()
+ {
+ Supplier messageSupplier = Mockito.mock(Supplier.class);
+ Mockito.when(messageSupplier.get()).thenReturn(EXPECTED_MESSAGE);
+
+ Log logger = new SystemStreamLog();
+ logger.debug(messageSupplier);
Review comment:
why only `logger.debug`
It is inconsistent with tests in `DefaultLogTest`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on pull request #438: [MNG-7083] - introduce lazy Log message evaluation
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #438:
URL: https://github.com/apache/maven/pull/438#issuecomment-767563022
>
>
> @McFoggy it is the outcome of the discussions because slf4j is 1. unstable, 2. has no correct binding impl (it is a singleton leading to a bunch of issues in mojo when transparently exposed) 3. does not enable to switch the binding until you reimpl slf4j by design 4. is not contextual with most available impl and 5. we already reimplement slf4j and just inherit from a small API part but not the rest. I fully understand it can be surprising but it is the outcome of most vendors in most projects caring of the context and to have a pluggable logging impl so it is not that shocking after some thoughts as seen in the discussion.
I would expect such discussions to be held with @ceki instead with just us.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] McFoggy commented on a change in pull request #438: [MNG-7083] - introduce lazy Log message evaluation
Posted by GitBox <gi...@apache.org>.
McFoggy commented on a change in pull request #438:
URL: https://github.com/apache/maven/pull/438#discussion_r564498644
##########
File path: maven-plugin-api/src/test/java/org/apache/maven/plugin/logging/SystemStreamLogTest.java
##########
@@ -0,0 +1,94 @@
+package org.apache.maven.plugin.logging;
+
+/*
+ * 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.
+ */
+
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+
+import java.io.PrintStream;
+import java.util.function.Supplier;
+
+/**
+ * Tests for {@link SystemStreamLog}
+ */
+public class SystemStreamLogTest
+{
+ private static final String EXPECTED_MESSAGE = "expected message";
+ private static PrintStream outStream;
+ private static PrintStream errStream;
+ private static PrintStream mockOut;
+ private static PrintStream mockErr;
+
+ /*
+ * As SystemStreamLog info/warn/error log levels are active, this test checks that
+ * a message supplier is really called/executed when logging at those levels
+ */
+ @Test
+ public void testLazyMessageIsEvaluatedForActiveLogLevels()
+ {
+ Supplier messageSupplier = Mockito.mock(Supplier.class);
+ Mockito.when(messageSupplier.get()).thenReturn(EXPECTED_MESSAGE);
+
+ Log logger = new SystemStreamLog();
+ logger.info(messageSupplier);
+ logger.warn(messageSupplier);
+ logger.error(messageSupplier);
+
+ Mockito.verify(messageSupplier, Mockito.times(3)).get();
+ }
+
+ /*
+ * As SystemStreamLog debug log level is inactive, this test checks that
+ * a message supplier is not called/executed when logging at debug level
+ */
+ @Test
+ public void testDebugLazyMessageIsNotEvaluated()
+ {
+ Supplier messageSupplier = Mockito.mock(Supplier.class);
+ Mockito.when(messageSupplier.get()).thenReturn(EXPECTED_MESSAGE);
+
+ Log logger = new SystemStreamLog();
+ logger.debug(messageSupplier);
Review comment:
same answer than above, log levels are hardcoded in `SystemStreamLog.java`.
Do you prefer the same kind of test than in `DefaultLogTest` ? meaning calling all log levels and check number of Supplier calls.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] McFoggy commented on a change in pull request #438: [MNG-7083] - introduce lazy Log message evaluation
Posted by GitBox <gi...@apache.org>.
McFoggy commented on a change in pull request #438:
URL: https://github.com/apache/maven/pull/438#discussion_r564504423
##########
File path: maven-plugin-api/pom.xml
##########
@@ -54,6 +54,11 @@ under the License.
<groupId>org.codehaus.plexus</groupId>
<artifactId>plexus-classworlds</artifactId>
</dependency>
+ <dependency>
Review comment:
That was just a simple unit test, as far as possible I didn't wanted to interfere/pollute the console outputs.
If it is not a common practice to mock then I can add a stub/custom impl (with control on log level) for the same purpose.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] rmannibucau commented on pull request #438: [MNG-7083] - introduce lazy Log message evaluation
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #438:
URL: https://github.com/apache/maven/pull/438#issuecomment-767550173
@McFoggy it is the outcome of the discussions because slf4j is 1. unstable, 2. has no correct binding impl (it is a singleton leading to a bunch of issues in mojo when transparently exposed) 3. does not enable to switch the binding until you reimpl slf4j by design 4. is not contextual with most available impl and 5. we already reimplement slf4j and just inherit from a small API part but not the rest. I fully understand it can be surprising but it is the outcome of most vendors in most projects caring of the context and to have a pluggable logging impl so it is not that shocking after some thoughts as seen in the discussion.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] rmannibucau commented on a change in pull request #438: [MNG-7083] - introduce lazy Log message evaluation
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #438:
URL: https://github.com/apache/maven/pull/438#discussion_r564558395
##########
File path: maven-plugin-api/src/main/java/org/apache/maven/plugin/logging/Log.java
##########
@@ -63,6 +65,20 @@
*/
void debug( Throwable error );
+ /**
+ * Send a message to the user in the <b>debug</b> error level by computing the message
+ * only when needed. The supplier will be called only if @see #isDebugEnabled() is <b>true</b>.
+ *
+ * @param messageSupplier a non null Supplier of the message to use
+ */
+ default void debug( Supplier<String> messageSupplier )
Review comment:
We had that discussion on Artifact#setPath/getpath thread and it ended up as a compromise to do both.
I'm not sure it is worth but I'm fine with this but implementing it in implementation is the prerequisite on my side - not having it in the interface is fine for me since it is not a breaking change until you upgrade your mojo API and have a custom impl - which is fine.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] McFoggy commented on a change in pull request #438: [MNG-7083] - introduce lazy Log message evaluation
Posted by GitBox <gi...@apache.org>.
McFoggy commented on a change in pull request #438:
URL: https://github.com/apache/maven/pull/438#discussion_r564571911
##########
File path: maven-plugin-api/src/main/java/org/apache/maven/plugin/logging/Log.java
##########
@@ -63,6 +65,20 @@
*/
void debug( Throwable error );
+ /**
+ * Send a message to the user in the <b>debug</b> error level by computing the message
+ * only when needed. The supplier will be called only if @see #isDebugEnabled() is <b>true</b>.
+ *
+ * @param messageSupplier a non null Supplier of the message to use
+ */
+ default void debug( Supplier<String> messageSupplier )
Review comment:
ok I'll remove the default methods from the interface
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] McFoggy commented on a change in pull request #438: [MNG-7083] - introduce lazy Log message evaluation
Posted by GitBox <gi...@apache.org>.
McFoggy commented on a change in pull request #438:
URL: https://github.com/apache/maven/pull/438#discussion_r564496153
##########
File path: maven-plugin-api/src/main/java/org/apache/maven/plugin/logging/Log.java
##########
@@ -63,6 +65,20 @@
*/
void debug( Throwable error );
+ /**
+ * Send a message to the user in the <b>debug</b> error level by computing the message
+ * only when needed. The supplier will be called only if @see #isDebugEnabled() is <b>true</b>.
+ *
+ * @param messageSupplier a non null Supplier of the message to use
+ */
+ default void debug( Supplier<String> messageSupplier )
Review comment:
I will add some for each log level with an exception.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] McFoggy commented on a change in pull request #438: [MNG-7083] - introduce lazy Log message evaluation
Posted by GitBox <gi...@apache.org>.
McFoggy commented on a change in pull request #438:
URL: https://github.com/apache/maven/pull/438#discussion_r564547259
##########
File path: maven-plugin-api/pom.xml
##########
@@ -54,6 +54,11 @@ under the License.
<groupId>org.codehaus.plexus</groupId>
<artifactId>plexus-classworlds</artifactId>
</dependency>
+ <dependency>
Review comment:
> with a custom Log impl
you probably meant with a org.codehaus.plexus.logging.Logger implementation ; applying to tests in DefaultLogTest.
@rmannibucau If I provide a stub/custom impl what is the difference with the mock ; I mean in regard of the remark _"... what it brings but I see how the test can pass with a broken impl"_
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] McFoggy commented on a change in pull request #438: [MNG-7083] - introduce lazy Log message evaluation
Posted by GitBox <gi...@apache.org>.
McFoggy commented on a change in pull request #438:
URL: https://github.com/apache/maven/pull/438#discussion_r564500288
##########
File path: maven-plugin-api/src/test/java/org/apache/maven/plugin/logging/SystemStreamLogTest.java
##########
@@ -0,0 +1,94 @@
+package org.apache.maven.plugin.logging;
+
+/*
+ * 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.
+ */
+
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+
+import java.io.PrintStream;
+import java.util.function.Supplier;
+
+/**
+ * Tests for {@link SystemStreamLog}
+ */
+public class SystemStreamLogTest
+{
+ private static final String EXPECTED_MESSAGE = "expected message";
+ private static PrintStream outStream;
+ private static PrintStream errStream;
+ private static PrintStream mockOut;
+ private static PrintStream mockErr;
+
+ /*
+ * As SystemStreamLog info/warn/error log levels are active, this test checks that
+ * a message supplier is really called/executed when logging at those levels
+ */
+ @Test
+ public void testLazyMessageIsEvaluatedForActiveLogLevels()
+ {
+ Supplier messageSupplier = Mockito.mock(Supplier.class);
+ Mockito.when(messageSupplier.get()).thenReturn(EXPECTED_MESSAGE);
+
+ Log logger = new SystemStreamLog();
+ logger.info(messageSupplier);
+ logger.warn(messageSupplier);
+ logger.error(messageSupplier);
+
+ Mockito.verify(messageSupplier, Mockito.times(3)).get();
+ }
+
+ /*
+ * As SystemStreamLog debug log level is inactive, this test checks that
+ * a message supplier is not called/executed when logging at debug level
+ */
+ @Test
+ public void testDebugLazyMessageIsNotEvaluated()
+ {
+ Supplier messageSupplier = Mockito.mock(Supplier.class);
+ Mockito.when(messageSupplier.get()).thenReturn(EXPECTED_MESSAGE);
+
+ Log logger = new SystemStreamLog();
+ logger.debug(messageSupplier);
+
+ Mockito.verify(messageSupplier, Mockito.never()).get();
+ }
+
+ @BeforeAll
+ public static void initialize()
+ {
+ outStream = System.out;
+ errStream = System.err;
+
+ mockOut = Mockito.mock(PrintStream.class);
+ System.setOut(mockOut);
+ mockErr = Mockito.mock(PrintStream.class);
Review comment:
@slawekjaranowski did you saw my [comment](https://github.com/apache/maven/pull/438#issuecomment-767480098) above ? I think I should remove totally the mocks for stdout/stderr since they can also interfere with others tests in parallel.
I think it is safer to pollute a bit the console output than to change stdout/stderr that are shared inside the JVM and could potentially be used while the test executes.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] McFoggy commented on pull request #438: [MNG-7083] - introduce lazy Log message evaluation
Posted by GitBox <gi...@apache.org>.
McFoggy commented on pull request #438:
URL: https://github.com/apache/maven/pull/438#issuecomment-767480098
Initially I did not wanted to pollute `stdout` & `stderr` that's why I mocked them in SystemStreamLogTest.
Thinking more about it, it can be a bad idea to have mocked them.
If required I'll remove the mock and let the SystemStreamLogTest test output messages.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org