[GitHub] [sling-org-apache-sling-resourceresolver] rombert commented on a change in pull request #53: SLING-10945 add metrics

[GitBox <gi...@apache.org>]

rombert commented on a change in pull request #53:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/53#discussion_r761946932



##########
File path: pom.xml
##########
@@ -149,6 +149,11 @@
             <artifactId>annotations</artifactId>
             <scope>provided</scope>
         </dependency>
+        <dependency>

Review comment:
       Can we make this dependency optional? I am worried that this will lead in a wave of dependency breakages in various consumer projects.

##########
File path: pom.xml
##########
@@ -149,6 +149,11 @@
             <artifactId>annotations</artifactId>
             <scope>provided</scope>
         </dependency>
+        <dependency>
+            <groupId>org.apache.sling</groupId>
+            <artifactId>org.apache.sling.commons.metrics</artifactId>
+            <version>1.2.8</version>

Review comment:
       The scope should be provided.

##########
File path: src/test/java/org/apache/sling/resourceresolver/impl/mapping/ResourceMapperImplTest.java
##########
@@ -94,6 +95,7 @@ public ResourceMapperImplTest(boolean optimiseAliasResolution) {
     @Before
     public void prepare() throws LoginException {
 
+        ctx.registerService(ResourceResolverMetrics.class, Mockito.mock(ResourceResolverMetrics.class));

Review comment:
       I think we shouldn't mix Sling mocks and mockito here. Can you just pass the plain object?




-- 
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: dev-unsubscribe@sling.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org