You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by "jvz (via GitHub)" <gi...@apache.org> on 2023/01/23 01:52:03 UTC
[GitHub] [logging-log4j2] jvz opened a new pull request, #1222: Use setters for builder classes
jvz opened a new pull request, #1222:
URL: https://github.com/apache/logging-log4j2/pull/1222
This also removes some related deprecated methods. Some builders have been updated to remove unnecessary extensibility when used in a final class.
This fixes #1206.
Signed-off-by: Matt Sicker <ma...@apache.org>
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on a diff in pull request #1222: Use setters for builder classes
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #1222:
URL: https://github.com/apache/logging-log4j2/pull/1222#discussion_r1083689526
##########
log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/LoggerConfigTest.java:
##########
@@ -1,3 +1,19 @@
+/*
+ * 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 org.apache.logging.log4j.core.config;/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
Review Comment:
One of the problems with Spotless: if a license is present **after** the package declaration, Spotless will insert another one.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/nosql/NoSqlAppender.java:
##########
@@ -74,9 +74,6 @@ public NoSqlAppender build() {
final NoSqlDatabaseManager<?> manager = NoSqlDatabaseManager.getNoSqlDatabaseManager(managerName,
bufferSize, provider);
- if (manager == null) {
- return null;
- }
Review Comment:
Shouldn't we keep this?
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/ConsoleAppender.java:
##########
@@ -130,19 +129,31 @@ public static <B extends Builder<B>> B newBuilder() {
@PluginBuilderAttribute
private boolean direct;
- public B setTarget(final Target aTarget) {
+ public Target getTarget() {
+ return target;
+ }
+
+ public boolean isFollow() {
+ return follow;
+ }
+
+ public boolean isDirect() {
+ return direct;
+ }
Review Comment:
Is this useful on the **builder**? I would prefer to see these on the `ConsoleAppender` itself (to be able to read the appender's configuration in runtime)
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] jvz commented on a diff in pull request #1222: Use setters for builder classes
Posted by "jvz (via GitHub)" <gi...@apache.org>.
jvz commented on code in PR #1222:
URL: https://github.com/apache/logging-log4j2/pull/1222#discussion_r1100582972
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/nosql/NoSqlAppender.java:
##########
@@ -74,9 +74,6 @@ public NoSqlAppender build() {
final NoSqlDatabaseManager<?> manager = NoSqlDatabaseManager.getNoSqlDatabaseManager(managerName,
bufferSize, provider);
- if (manager == null) {
- return null;
- }
Review Comment:
IntelliJ's static code analysis noted that `manager` is never `null`.
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] jvz commented on a diff in pull request #1222: Use setters for builder classes
Posted by "jvz (via GitHub)" <gi...@apache.org>.
jvz commented on code in PR #1222:
URL: https://github.com/apache/logging-log4j2/pull/1222#discussion_r1100583832
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/ConsoleAppender.java:
##########
@@ -130,19 +129,31 @@ public static <B extends Builder<B>> B newBuilder() {
@PluginBuilderAttribute
private boolean direct;
- public B setTarget(final Target aTarget) {
+ public Target getTarget() {
+ return target;
+ }
+
+ public boolean isFollow() {
+ return follow;
+ }
+
+ public boolean isDirect() {
+ return direct;
+ }
Review Comment:
Have added them for consistency. This is more useful when you extend the builder in another builder class and need access to its state to build something.
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Use setters for builder classes (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy closed pull request #1222: Use setters for builder classes
URL: https://github.com/apache/logging-log4j2/pull/1222
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] garydgregory commented on a diff in pull request #1222: Use setters for builder classes
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #1222:
URL: https://github.com/apache/logging-log4j2/pull/1222#discussion_r1083872749
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/nosql/NoSqlAppender.java:
##########
@@ -74,9 +74,6 @@ public NoSqlAppender build() {
final NoSqlDatabaseManager<?> manager = NoSqlDatabaseManager.getNoSqlDatabaseManager(managerName,
bufferSize, provider);
- if (manager == null) {
- return null;
- }
Review Comment:
It must be there for a reason but we don't have a unit test that fails without it maybe. It might have to do with the reconnection logic which can be hard to 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.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vy commented on pull request #1222: Use setters for builder classes
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on PR #1222:
URL: https://github.com/apache/logging-log4j2/pull/1222#issuecomment-1420659210
@jvz, this has been reviewed by 2 people already. Would you please address reviewers' remarks and if possible, proceed with merging this PR and consequently closing the ticket, please?
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org