-
Notifications
You must be signed in to change notification settings - Fork 464
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Using Maven dependency mediation #296
Conversation
@nedtwigg As stated above, currently the problem has no effect on the behaviour. Do you anyhow want an update of the Sorry about the commit message. Will change it when merging. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a minor suggestion for making the tests a little easier to read, but the substance of the change looks good to me.
Set<File> files = provisioner.provisionWithDependencies("foo", "bar", "baz"); | ||
|
||
assertThat(files).containsOnly(new File("foo-1"), new File("foo-2"), new File("baz-2")); | ||
private void resolveDependencies() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was a little confused about what these tests were doing at first. Maybe rename this to assertFormattingWorks
. It would also be a little more robust if it didn't just run, but also asserted that the file on disk was as expected. Maybe assertFormattingWorks(String unformatted, String formatted)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Made a couple tiny comments.
plugin-maven/src/main/java/com/diffplug/spotless/maven/ArtifactResolver.java
Show resolved
Hide resolved
return mavenCoords -> mavenCoords.stream() | ||
.flatMap(coord -> artifactResolver.resolve(coord).stream()) | ||
.collect(toSet()); | ||
return mavenCoords -> artifactResolver.resolve(mavenCoords); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe artifactResolver::resolve
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lutovich Sorry, maybe it's just to late... I lost you. Can you explain in detail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just mean that lambda
return mavenCoords -> artifactResolver.resolve(mavenCoords);
can probably be replaced with method reference
return artifactResolver::resolve;
Then Objects.requireNonNull(artifactResolver);
can also be removed because method ref will fail with NPE anyway.
Currently Spotless uses 2 kinds of dependencies:
The
Provisioner
interface supports "dependency lists" (currently always resolving transitives).The previous
MavenProvisioner
implementation took individually each dependency as a root dependency, resolves it (and its transitives) and puts the JARs into a hash set.There had been quite an evolution how Maven resolves transitives. Please refer to the Transitive Dependencies documentation for details.
The whole issue is NOT affecting the current formatters (due to the simple dependency structure of current formatters). However, the
maven-plugin
Provisioner
should not interfere with the dependency resolution ofMaven
to avoid bugs in future evolution ofSpotless
.