[MDEP-645] Created new mojo GetClassesMojo which lists all class dependencies for a specified artifact. #57
Conversation
|
First please create an appropriate JIRA issue for that and describe the use case for this. |
jonvolfson
changed the title
|
Updated PR title. This is a fix for MDEP-645 |
| * | ||
| * @since 2.7 | ||
| */ | ||
| @Parameter( property = "mdep.skip", defaultValue = "false" ) |
There was a problem hiding this comment.
I think you can remove all of the 'skip' processing; it's not terribly useful for a mojo intended for CLI.
There was a problem hiding this comment.
@bimargulies-google I'd keep it since you never know how it is used and maven does not control that
| public ProjectBuildingRequest buildBuildingRequest() | ||
| throws MojoExecutionException, MojoFailureException | ||
| { | ||
| if ( coordinate.getArtifactId() == null && artifact == null ) |
There was a problem hiding this comment.
You don't use coordinate below. Are you missing an else clause? Is that the code that would run if someone used -DgroupId ... instead of -Dartifact?
There was a problem hiding this comment.
Fixed. Removed the coordinate.getArtifactId() part. This was recycled code from the GetMojo with a similar use case.
|
|
||
| for ( ArtifactResult result : artifacts ) | ||
| { | ||
| printClassesFromArtifactResult( result ); |
There was a problem hiding this comment.
Print the file name of the jar before printing its contents if you are printing more than one jar file's worth?
| setVariableValueToObject( mojo, "transitive", Boolean.FALSE ); | ||
|
|
||
| mojo.execute(); | ||
| return; |
| setVariableValueToObject( mojo, "transitive", Boolean.TRUE ); | ||
|
|
||
| mojo.execute(); | ||
| return; |
| return; | ||
| } | ||
|
|
||
| public void testGetClassesTransitive() |
There was a problem hiding this comment.
Um, this does not seem to test anything except that it does not explode.
There was a problem hiding this comment.
When listing classes for something like org.apache.maven:maven-model:2.0.9, setting -Dtransitive=true lists more classes than the false alternative. It's possible that the artifact used in the test has little/no transitive dependencies which means the results would be similar/the same.
|
|
||
| // remove .class from the end and change format to use periods instead of forward slashes | ||
| name = name.substring( 0, name.length() - 6 ).replace( '/', '.' ); | ||
| getLog().info( name ); |
There was a problem hiding this comment.
I don't think that using the log is the best delivery mechanism, but I could be confused. @elharo what do you think? Also, I have no idea how to test for log output, you'd need to mock the log somehow.
There was a problem hiding this comment.
+1, can be neat to be able to inject the list of classes in a maven project properties to reuse it downstream (exec-maven-plugin is my immediate thought but i'm sure there are more use cases). A dump in a file can be useful too.
side note: what about multi release jars (META-INF/versions)? should it be filtered?
There was a problem hiding this comment.
I think logging is how we usually do this. The log can be injected or tested.
…recycled code for building the ProjectBuildRequest
|
|
||
|
|
||
| /** | ||
| * Retrieves and lists all class dependencies for the specified artifact from the specified remote repositories. |
There was a problem hiding this comment.
class dependencies for --> "classes contained in" as these are not dependencies
| * Retrieves and lists all class dependencies for the specified artifact from the specified remote repositories. | ||
| */ | ||
| @Mojo( name = "get-classes", requiresProject = false, threadSafe = true ) | ||
| public class GetClassesMojo |
| private DefaultDependableCoordinate coordinate = new DefaultDependableCoordinate(); | ||
|
|
||
| /** | ||
| * The groupId of the artifact to download. Ignored if {@link #artifact} is used. |
| private String groupId; | ||
|
|
||
| /** | ||
| * The artifactId of the artifact to download. Ignored if {@link #artifact} is used. |
| * The packaging of the artifact to download. Ignored if {@link #artifact} is used. | ||
| */ | ||
| @Parameter( property = "packaging", defaultValue = "jar" ) | ||
| private String packaging = "jar"; |
There was a problem hiding this comment.
will this work for anything that's not a jar?
There was a problem hiding this comment.
It will work for a .war or any other 'renamed jar'. But the substitution of dots and slashes should perhaps be removed for non-.jar.
| } | ||
|
|
||
| /** | ||
| * @param artifact The artifact |
There was a problem hiding this comment.
doc comments begin with lower case, per Oracle guidelines
There was a problem hiding this comment.
More clarification? This is taken from the GetMojo.
| } | ||
|
|
||
| /** | ||
| * @param artifactId The artifactId. |
| import org.apache.maven.plugin.testing.stubs.MavenProjectStub; | ||
| import org.apache.maven.settings.Server; | ||
| import org.apache.maven.settings.Settings; | ||
| import org.sonatype.aether.impl.internal.SimpleLocalRepositoryManager; |
There was a problem hiding this comment.
Tricky but can we replace this with org.eclipse equivalents? Probably not a 1:1 replacement.
There was a problem hiding this comment.
Do you have an example to point at here?
There was a problem hiding this comment.
Not off the top of my head but try calling lookup(LocalRepositoryManager.class) in the setUp method and see if that returns an object that works.
| public class TestGetClassesMojo | ||
| extends AbstractDependencyMojoTestCase | ||
| { | ||
| GetClassesMojo mojo; |
There was a problem hiding this comment.
private is not allowed for this test class
| super.setUp( "markers", false ); | ||
|
|
||
| File testPom = new File( getBasedir(), "target/test-classes/unit/get-test/plugin-config.xml" ); | ||
| assert testPom.exists(); |
…ansitive default value to false, changed ListClassesMojo methods to private if able, replaced assert in test case.
| private String packaging = "jar"; | ||
|
|
||
| /** | ||
| * Repositories in the format id::[layout]::url or just URLs, separated by comma. ie. |
There was a problem hiding this comment.
avoid Latin. ie. --> That is,
| return dependencyResolver; | ||
| } | ||
|
|
||
| protected ArtifactResolver getArtifactResolver() |
There was a problem hiding this comment.
fixed, removed this method and other getters that weren't needed
| { | ||
| if ( isTransitive() ) | ||
| { | ||
| Iterable<ArtifactResult> artifacts = getDependencyResolver() |
There was a problem hiding this comment.
you can access fields like dependencyResolver directly. There's no need to use a getter method in the same class.
| public void execute() throws MojoExecutionException, MojoFailureException | ||
| { | ||
| ProjectBuildingRequest buildingRequest = buildBuildingRequest(); | ||
| DefaultDependableCoordinate coordinate = getCoordinate(); |
There was a problem hiding this comment.
don't shadow the field here
| } | ||
| catch ( ArtifactResolverException | DependencyResolverException | IOException e ) | ||
| { | ||
| throw new MojoExecutionException( "Couldn't download artifact: " + e.getMessage(), e ); |
There was a problem hiding this comment.
exception message should contain coordinates of artifact it was trying to download
| jarFile.close(); | ||
| } | ||
|
|
||
| private ProjectBuildingRequest buildBuildingRequest() |
| } | ||
|
|
||
| // remove .class from the end and change format to use periods instead of forward slashes | ||
| name = name.substring( 0, name.length() - 6 ).replace( '/', '.' ); |
There was a problem hiding this comment.
try to avoid reusing local variables. Here you have two things: an entry name and a class name, so two different variables make this clearer
| } | ||
|
|
||
| /** | ||
| * @param artifact The artifact. |
There was a problem hiding this comment.
…eract with the mojo, fixed comments to follow oracle javadoc guidelines, changes method names to be more descriptive of their purpose, fixed shadowing variable names and renamed variables to be more descriptive
|
|
||
| /** | ||
| * Skip plugin execution completely. | ||
| * |
| } | ||
| catch ( ArtifactResolverException | DependencyResolverException | IOException e ) | ||
| { | ||
| throw new MojoExecutionException( "Couldn't download artifact " + artifact + ": " + e.getMessage(), e ); |
There was a problem hiding this comment.
This message might not be quite accurate when you're downloading all transitive dependencies. That is, it could be one of the transitive deps we can't download instead of the original.
There was a problem hiding this comment.
I believe the original message was correct before I changed it to include artifact. If a transitive dependency wasn't able to be resolved, e.getMessage() should have included the correct one.
| String className = entryName.substring( 0, entryName.length() - 6 ).replace( '/', '.' ); | ||
| getLog().info( className ); | ||
| } | ||
| jarFile.close(); |
There was a problem hiding this comment.
use try-with-resources to guarantee that this is closed when an exception is thrown
| } | ||
|
|
||
| /** | ||
| * @return {@link #skip} |
There was a problem hiding this comment.
I don't think you need these getter methods, and probably not the setters
| public class TestGetClassesMojo | ||
| extends AbstractDependencyMojoTestCase | ||
| { | ||
| ListClassesMojo mojo; |
…test class a private field, included printing logic to use a try-with-resource statement to guarantee JarFile closure, reverted thrown error message when resolving dependencies to original to be less confusing.
…ndencies for a specified artifact. (apache#57) * Created new mojo GetClassesMojo which lists all class dependencies for a specified artifact * adding test file and fixing small function name * Updating test functions names to be self-describing * Fixing styling issues preventing build completion * Removed return statements from test cases, removed unneeded piece of recycled code for building the ProjectBuildRequest * Changed GetClassesMojo to ListClassesMojo, fixed comments, changed transitive default value to false, changed ListClassesMojo methods to private if able, replaced assert in test case. * Changed modifying methods to private since outside classes do not interact with the mojo, fixed comments to follow oracle javadoc guidelines, changes method names to be more descriptive of their purpose, fixed shadowing variable names and renamed variables to be more descriptive * Removed unneccessary setter and getter methods, made the mojo in the test class a private field, included printing logic to use a try-with-resource statement to guarantee JarFile closure, reverted thrown error message when resolving dependencies to original to be less confusing.
…ndencies for a specified artifact. (apache#57) * Created new mojo GetClassesMojo which lists all class dependencies for a specified artifact * adding test file and fixing small function name * Updating test functions names to be self-describing * Fixing styling issues preventing build completion * Removed return statements from test cases, removed unneeded piece of recycled code for building the ProjectBuildRequest * Changed GetClassesMojo to ListClassesMojo, fixed comments, changed transitive default value to false, changed ListClassesMojo methods to private if able, replaced assert in test case. * Changed modifying methods to private since outside classes do not interact with the mojo, fixed comments to follow oracle javadoc guidelines, changes method names to be more descriptive of their purpose, fixed shadowing variable names and renamed variables to be more descriptive * Removed unneccessary setter and getter methods, made the mojo in the test class a private field, included printing logic to use a try-with-resource statement to guarantee JarFile closure, reverted thrown error message when resolving dependencies to original to be less confusing.
|
Resolve #1075 |
5 participants
@elharo @bimargulies