[GitHub] [maven-dependency-analyzer] elharo commented on a change in pull request #14: [MDEP-708] - dependency:analyze recommends test scope for test-only artifacts that have non-test scope

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

elharo commented on a change in pull request #14:
URL: https://github.com/apache/maven-dependency-analyzer/pull/14#discussion_r452947777



##########
File path: src/main/java/org/apache/maven/shared/dependency/analyzer/DefaultProjectDependencyAnalyzer.java
##########
@@ -189,6 +210,29 @@ else if ( file != null && file.isDirectory() )
         return artifactClassMap;
     }
 
+    protected Set<String> buildTestDependencyClasses( MavenProject project ) throws IOException

Review comment:
       
   
   can this be private or package protected?
   

##########
File path: src/main/java/org/apache/maven/shared/dependency/analyzer/DefaultProjectDependencyAnalyzer.java
##########
@@ -129,6 +135,21 @@ public ProjectDependencyAnalysis analyze( MavenProject project )
         return results;
     }
 
+    protected Set<Artifact> getTestArtifactsWithNonTestScope( Set<Artifact> testOnlyArtifacts )

Review comment:
       can this be private or package protected?

##########
File path: src/main/java/org/apache/maven/shared/dependency/analyzer/DefaultProjectDependencyAnalyzer.java
##########
@@ -189,6 +210,29 @@ else if ( file != null && file.isDirectory() )
         return artifactClassMap;
     }
 
+    protected Set<String> buildTestDependencyClasses( MavenProject project ) throws IOException
+    {
+        Set<String> nonTestDependencyClasses = new HashSet<String>();

Review comment:
       Can just write `new HashSet<>` without an explicit generic type

##########
File path: src/main/java/org/apache/maven/shared/dependency/analyzer/ProjectDependencyAnalysis.java
##########
@@ -88,6 +92,15 @@ public ProjectDependencyAnalysis( Set<Artifact> usedDeclaredArtifacts, Set<Artif
         return unusedDeclaredArtifacts;
     }
 
+    /**
+     * Test Artifacts that have a non-test scope
+     * @return {@link Artifact}
+     */
+    public Set<Artifact> getTestArtifactsWithNonTestScope()
+    {
+        return testArtifactsWithNonTestScope;

Review comment:
       exposes mutable internal state; a defensive copy might be appropriate

##########
File path: src/main/java/org/apache/maven/shared/dependency/analyzer/ProjectDependencyAnalysis.java
##########
@@ -44,19 +44,23 @@
 
     private final Set<Artifact> unusedDeclaredArtifacts;
 
+    private final Set<Artifact> testArtifactsWithNonTestScope;
+
     // constructors -----------------------------------------------------------
 
     public ProjectDependencyAnalysis()
     {
-        this( null, null, null );
+        this( null, null, null, null );
     }
 
     public ProjectDependencyAnalysis( Set<Artifact> usedDeclaredArtifacts, Set<Artifact> usedUndeclaredArtifacts,

Review comment:
       adding an argument incompatibly changes the existing API; consider an additional constructor instead

##########
File path: src/test/java/org/apache/maven/shared/dependency/analyzer/DefaultProjectDependencyAnalyzerTest.java
##########
@@ -267,12 +271,36 @@ public void testJarWithXmlTransitiveDependency()
         Artifact jdom = createArtifact( "dom4j", "dom4j", "jar", "1.6.1", "compile" );
         Set<Artifact> usedDeclaredArtifacts = Collections.singleton( jdom );
 
-        ProjectDependencyAnalysis expectedAnalysis = new ProjectDependencyAnalysis( usedDeclaredArtifacts, null, null );
+        ProjectDependencyAnalysis expectedAnalysis = new ProjectDependencyAnalysis( usedDeclaredArtifacts, null, null,
+                null );
 
         // MSHARED-47: usedUndeclaredArtifacts=[xml-apis:xml-apis:jar:1.0.b2:compile]
         // assertEquals( expectedAnalysis, actualAnalysis );
     }
 
+    public void testJarWithNonTestScopedTestDependency()
+            throws TestToolsException, ProjectDependencyAnalyzerException
+    {
+        compileProject( "jarWithNonTestScopedTestDependency/pom.xml" );
+
+        MavenProject project2 = getProject( "jarWithNonTestScopedTestDependency/project2/pom.xml" );
+
+        ProjectDependencyAnalysis actualAnalysis = analyzer.analyze( project2 );
+
+        Artifact project1 = createArtifact( "org.apache.maven.shared.dependency-analyzer.tests",

Review comment:
       artifact1?

##########
File path: src/test/resources/jarWithNonTestScopedTestDependency/pom.xml
##########
@@ -0,0 +1,38 @@
+<?xml version="1.0" encoding="UTF-8"?>
+
+<!--
+  ~ 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.
+  -->
+
+<project
+	xmlns="http://maven.apache.org/POM/4.0.0"
+	xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+	xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd"
+>
+	<modelVersion>4.0.0</modelVersion>
+	<groupId>org.apache.maven.shared.dependency-analyzer.tests</groupId>
+	<artifactId>jarWithNonTestScopedTestDependency</artifactId>
+	<packaging>pom</packaging>

Review comment:
       2 space indents in XML




----------------------------------------------------------------
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