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 2020/07/10 20:31:49 UTC
[GitHub] [maven-dependency-plugin] ian-lavallee opened a new pull request #80: [MDEP-708] [NEEDS Maven-dependency-analyzer RELEASE] dependency:analyze should recommend narrower scope where possible
ian-lavallee opened a new pull request #80:
URL: https://github.com/apache/maven-dependency-plugin/pull/80
This can't be merged until we have a new release and final version of maven-dependency-analyzer. For now it relies on a snapshot version.
----------------------------------------------------------------
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-dependency-plugin] ian-lavallee edited a comment on pull request #80: [MDEP-708] dependency:analyze should recommend test scope where possible
Posted by GitBox <gi...@apache.org>.
ian-lavallee edited a comment on pull request #80:
URL: https://github.com/apache/maven-dependency-plugin/pull/80#issuecomment-662560710
Note because of the last commit relying on the new dependency-analyzer we need 1.11.3-SNAPSHOT for the analyzer now.
----------------------------------------------------------------
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-dependency-plugin] ian-lavallee commented on a change in pull request #80: [MDEP-708] dependency:analyze should recommend test scope where possible
Posted by GitBox <gi...@apache.org>.
ian-lavallee commented on a change in pull request #80:
URL: https://github.com/apache/maven-dependency-plugin/pull/80#discussion_r458309097
##########
File path: src/main/java/org/apache/maven/plugins/dependency/analyze/AbstractAnalyzeMojo.java
##########
@@ -49,7 +49,7 @@
/**
* Analyzes the dependencies of this project and determines which are: used and declared; used and undeclared; unused
- * and declared. Also determines which dependencies have a broader scope than needed.
+ * and declared; non-test scoped test-only dependencies.
Review comment:
Ok, the test fails if it's scoped with runtime but used in non test classes but it seems like it may be failing for the wrong reason. Gonna look into this more. If it needs to ignore runtime scopes that shouldn't be too complicated but will need to be changed in the actual dependency-analyzer project.
----------------------------------------------------------------
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-dependency-plugin] elharo commented on a change in pull request #80: [MDEP-708] dependency:analyze should recommend test scope where possible
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #80:
URL: https://github.com/apache/maven-dependency-plugin/pull/80#discussion_r458303008
##########
File path: src/main/java/org/apache/maven/plugins/dependency/analyze/AbstractAnalyzeMojo.java
##########
@@ -49,7 +49,7 @@
/**
* Analyzes the dependencies of this project and determines which are: used and declared; used and undeclared; unused
- * and declared. Also determines which dependencies have a broader scope than needed.
+ * and declared; non-test scoped test-only dependencies.
Review comment:
That sounds like an issue. We probably don't want to suggest changing runtime scope to test scope since the analyzer can't detect how a runtime dependency is used.
----------------------------------------------------------------
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-dependency-plugin] elharo commented on pull request #80: [MDEP-708] dependency:analyze should recommend test scope where possible
Posted by GitBox <gi...@apache.org>.
elharo commented on pull request #80:
URL: https://github.com/apache/maven-dependency-plugin/pull/80#issuecomment-662593053
No, just be aware
----------------------------------------------------------------
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-dependency-plugin] bimargulies-google commented on a change in pull request #80: [MDEP-708] dependency:analyze should recommend test scope where possible
Posted by GitBox <gi...@apache.org>.
bimargulies-google commented on a change in pull request #80:
URL: https://github.com/apache/maven-dependency-plugin/pull/80#discussion_r457525286
##########
File path: pom.xml
##########
@@ -427,14 +428,18 @@ under the License.
<pomExclude>tree-verbose/pom.xml</pomExclude>
</pomExcludes>
<pomIncludes>
+<!--
Review comment:
Delete it if we no longer want it.
##########
File path: src/main/java/org/apache/maven/plugins/dependency/analyze/AbstractAnalyzeMojo.java
##########
@@ -357,6 +359,15 @@ private boolean checkDependencies()
warning = true;
}
+ if ( !testArtifactsWithNonTestScope.isEmpty() )
+ {
+ getLog().warn( "Non-test scoped test only dependencies found:" );
Review comment:
Is this the message for 'have a broader scope' in the comment above? If so, perhaps change the comment to read more specifically that you are detecting "non-test scoped test-only dependencies'.
----------------------------------------------------------------
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-dependency-plugin] elharo commented on a change in pull request #80: [MDEP-708] dependency:analyze should recommend test scope where possible
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #80:
URL: https://github.com/apache/maven-dependency-plugin/pull/80#discussion_r456592396
##########
File path: pom.xml
##########
@@ -222,7 +222,8 @@ under the License.
<dependency>
<groupId>org.apache.maven.shared</groupId>
<artifactId>maven-dependency-analyzer</artifactId>
- <version>1.11.1</version>
+ <!-- NEED TO WAIT FOR RELEASE -->
+ <version>1.11.2-SNAPSHOT</version>
Review comment:
given that we can depend on snapshot versions, are you comfortable with this PR being merged now?
----------------------------------------------------------------
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-dependency-plugin] ian-lavallee commented on pull request #80: [MDEP-708] dependency:analyze should recommend test scope where possible
Posted by GitBox <gi...@apache.org>.
ian-lavallee commented on pull request #80:
URL: https://github.com/apache/maven-dependency-plugin/pull/80#issuecomment-665743730
This PR needs a 1.11.3-SNAPSHOT dependency-analyzer so that the test for compileScopedTestDependencies passes. Currently it fails because in 1.11.2 the dependency-analyzer considers runtime dependencies for testDependency analysis when it shouldn't
----------------------------------------------------------------
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-dependency-plugin] ian-lavallee commented on pull request #80: [MDEP-708] dependency:analyze should recommend test scope where possible
Posted by GitBox <gi...@apache.org>.
ian-lavallee commented on pull request #80:
URL: https://github.com/apache/maven-dependency-plugin/pull/80#issuecomment-669277156
@elharo can you trigger a CI build for this branch?
----------------------------------------------------------------
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-dependency-plugin] ian-lavallee commented on a change in pull request #80: [MDEP-708] dependency:analyze should recommend test scope where possible
Posted by GitBox <gi...@apache.org>.
ian-lavallee commented on a change in pull request #80:
URL: https://github.com/apache/maven-dependency-plugin/pull/80#discussion_r458300094
##########
File path: src/main/java/org/apache/maven/plugins/dependency/analyze/AbstractAnalyzeMojo.java
##########
@@ -49,7 +49,7 @@
/**
* Analyzes the dependencies of this project and determines which are: used and declared; used and undeclared; unused
- * and declared. Also determines which dependencies have a broader scope than needed.
+ * and declared; non-test scoped test-only dependencies.
Review comment:
It also works if a test only dependency is scoped with runtime, provided, or compile so how about; runtime, provided, or compile scoped but only used in tests?
----------------------------------------------------------------
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-dependency-plugin] elharo commented on a change in pull request #80: [MDEP-708] [NEEDS Maven-dependency-analyzer RELEASE] dependency:analyze should recommend narrower scope where possible
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #80:
URL: https://github.com/apache/maven-dependency-plugin/pull/80#discussion_r455948255
##########
File path: pom.xml
##########
@@ -222,7 +222,8 @@ under the License.
<dependency>
<groupId>org.apache.maven.shared</groupId>
<artifactId>maven-dependency-analyzer</artifactId>
- <version>1.11.1</version>
+ <!-- NEED TO WAIT FOR RELEASE -->
+ <version>1.11.2-SNAPSHOT</version>
Review comment:
I've pushed this version to the snapshots repo. I'm checking on whether we can commit this based on that. I see at least one other plugin repo depending on a snapshot version.
----------------------------------------------------------------
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-dependency-plugin] elharo merged pull request #80: [MDEP-708] dependency:analyze should recommend test scope where possible
Posted by GitBox <gi...@apache.org>.
elharo merged pull request #80:
URL: https://github.com/apache/maven-dependency-plugin/pull/80
----------------------------------------------------------------
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-dependency-plugin] elharo commented on a change in pull request #80: [MDEP-708] dependency:analyze should recommend test scope where possible
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #80:
URL: https://github.com/apache/maven-dependency-plugin/pull/80#discussion_r458288444
##########
File path: src/main/java/org/apache/maven/plugins/dependency/analyze/AbstractAnalyzeMojo.java
##########
@@ -49,7 +49,7 @@
/**
* Analyzes the dependencies of this project and determines which are: used and declared; used and undeclared; unused
- * and declared. Also determines which dependencies have a broader scope than needed.
+ * and declared; non-test scoped test-only dependencies.
Review comment:
non-test scoped test-only dependencies --> compile scoped but only used in tests
----------------------------------------------------------------
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-dependency-plugin] ian-lavallee commented on a change in pull request #80: [MDEP-708] dependency:analyze should recommend test scope where possible
Posted by GitBox <gi...@apache.org>.
ian-lavallee commented on a change in pull request #80:
URL: https://github.com/apache/maven-dependency-plugin/pull/80#discussion_r456600762
##########
File path: pom.xml
##########
@@ -222,7 +222,8 @@ under the License.
<dependency>
<groupId>org.apache.maven.shared</groupId>
<artifactId>maven-dependency-analyzer</artifactId>
- <version>1.11.1</version>
+ <!-- NEED TO WAIT FOR RELEASE -->
+ <version>1.11.2-SNAPSHOT</version>
Review comment:
Yes but I'm hoping to get @bimargulies to review it first so he has more of my work to base his feedback off
----------------------------------------------------------------
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-dependency-plugin] ian-lavallee commented on a change in pull request #80: [MDEP-708] dependency:analyze should recommend test scope where possible
Posted by GitBox <gi...@apache.org>.
ian-lavallee commented on a change in pull request #80:
URL: https://github.com/apache/maven-dependency-plugin/pull/80#discussion_r458288386
##########
File path: pom.xml
##########
@@ -222,7 +222,8 @@ under the License.
<dependency>
<groupId>org.apache.maven.shared</groupId>
<artifactId>maven-dependency-analyzer</artifactId>
- <version>1.11.1</version>
+ <!-- NEED TO WAIT FOR RELEASE -->
+ <version>1.11.2-SNAPSHOT</version>
Review comment:
@elharo The PR can be merged now
----------------------------------------------------------------
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-dependency-plugin] elharo commented on pull request #80: [MDEP-708] dependency:analyze should recommend test scope where possible
Posted by GitBox <gi...@apache.org>.
elharo commented on pull request #80:
URL: https://github.com/apache/maven-dependency-plugin/pull/80#issuecomment-662566105
FYI, I'm thinking the next release of maven-dependency-analyzer perhaps should be 1.12.0.
----------------------------------------------------------------
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-dependency-plugin] ian-lavallee commented on pull request #80: [MDEP-708] dependency:analyze should recommend test scope where possible
Posted by GitBox <gi...@apache.org>.
ian-lavallee commented on pull request #80:
URL: https://github.com/apache/maven-dependency-plugin/pull/80#issuecomment-662560710
Not because of the last commit relying on the new dependency-analyzer we need 1.11.3-SNAPSHOT for the analyzer now.
----------------------------------------------------------------
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-dependency-plugin] ian-lavallee commented on pull request #80: [MDEP-708] [NEEDS Maven-dependency-analyzer RELEASE] dependency:analyze should recommend narrower scope where possible
Posted by GitBox <gi...@apache.org>.
ian-lavallee commented on pull request #80:
URL: https://github.com/apache/maven-dependency-plugin/pull/80#issuecomment-656877067
The test relies on the line count of the HTML file but I'm going to change that to verify the contents and just look for Junit in the NonTestScopedTestDependency section. Just wanted to get the PR in for you to see.
----------------------------------------------------------------
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-dependency-plugin] ian-lavallee commented on pull request #80: [MDEP-708] dependency:analyze should recommend test scope where possible
Posted by GitBox <gi...@apache.org>.
ian-lavallee commented on pull request #80:
URL: https://github.com/apache/maven-dependency-plugin/pull/80#issuecomment-662583514
Should I change it to depend on 1.12.0 even though that's not released now then?
----------------------------------------------------------------
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-dependency-plugin] ian-lavallee commented on pull request #80: [MDEP-708] [NEEDS Maven-dependency-analyzer RELEASE] dependency:analyze should recommend narrower scope where possible
Posted by GitBox <gi...@apache.org>.
ian-lavallee commented on pull request #80:
URL: https://github.com/apache/maven-dependency-plugin/pull/80#issuecomment-656876714
@elharo
----------------------------------------------------------------
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