ctubbsii commented on code in PR #2155:
URL: https://github.com/apache/zookeeper/pull/2155#discussion_r1559857699
##########
zookeeper-server/pom.xml:
##########
@@ -177,6 +169,11 @@
<artifactId>tools</artifactId>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>ch.qos.logback</groupId>
+ <artifactId>logback-classic</artifactId>
+ <scope>test</scope>
+ </dependency>
Review Comment:
There are two requirements here:
1. Ensure it's available for the tests, which require it, and
2. Ensure it is excluded by default from any project that depends on this
one.
Specifying it as a test scope achieves both of these. And, because runtime
scope is included in the test classpath, specifying it as an optional runtime
dependency would also achieve both of these.
However, specifying an optional runtime dependency here would be a bit more
helpful, because users generally familiar with Java logging frameworks would
understand that there's a runtime component that they need to add themselves.
This is not clear when it's only added to the test scope.
##########
zookeeper-assembly/pom.xml:
##########
@@ -103,6 +103,10 @@
<groupId>jline</groupId>
<artifactId>jline</artifactId>
</dependency>
+ <dependency>
+ <groupId>ch.qos.logback</groupId>
+ <artifactId>logback-classic</artifactId>
+ </dependency>
Review Comment:
This is good because it actually is an explicit dependency of the assembly.
This project requires it in the bundle it creates. So, regardless of what
happens in the other pom.xml, this is correct.
--
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-unsubscr...@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
