Skip to content
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

Resubmit #149 with fixes #182

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,12 @@
<artifactId>scala-library</artifactId>
<version>${version.scala.major}${version.scala.minor}</version>
</dependency>
<dependency>
<groupId>org.apache.maven</groupId>
<artifactId>maven-core</artifactId>
<version>${version.maven}</version>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>org.apache.maven</groupId>
<artifactId>maven-plugin-api</artifactId>
Expand Down Expand Up @@ -127,12 +133,6 @@
<version>${version.scalatest}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.apache.maven</groupId>
<artifactId>maven-project</artifactId>
<version>2.2.1</version>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>org.scala-lang.modules</groupId>
<artifactId>scala-xml_${version.scala.major}</artifactId>
Expand Down
48 changes: 31 additions & 17 deletions src/main/java/org/antipathy/mvn_scalafmt/FormatMojo.java
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import org.apache.maven.model.Repository;

import java.io.File;
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;

Expand All @@ -25,9 +26,9 @@ public class FormatMojo extends AbstractMojo {
private boolean skipTestSources;
@Parameter(property = "format.skipSources", defaultValue = "false")
private boolean skipSources;
@Parameter(defaultValue = "${project.build.sourceDirectory}/../scala", required = true)
@Parameter()
private List<File> sourceDirectories;
@Parameter(defaultValue = "${project.build.testSourceDirectory}/../scala", required = true)
@Parameter()
private List<File> testSourceDirectories;
@Parameter(property = "format.validateOnly", defaultValue = "false")
private boolean validateOnly;
Expand All @@ -43,12 +44,10 @@ public class FormatMojo extends AbstractMojo {
* then we consider the value in {@code <branch>} tag as a command to run and the output will be used as the actual branch */
@Parameter(property = "format.branch", defaultValue = "master")
private String branch;
@Parameter(readonly = true, defaultValue = "${project}")
@Parameter(property = "project", readonly = true, required = true, defaultValue = "${project}")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

compared to #149: added property = "project" as some online sources insist that it's required with the switch from maven-project to maven-core.

however, i also tested with setting only one of property = "project" and defaultValue = "${project}", and all three versions seem to work. looks like this was not the culprit, but decided to keep both just in case.

private MavenProject project;
@Parameter(property = "format.useSpecifiedRepositories", defaultValue = "false")
private boolean useSpecifiedRepositories;
@Parameter(readonly = true, defaultValue = "${project.repositories}")
private List<Repository> mavenRepositories;

private List<String> getRepositoriesUrls(List<Repository> repositories) {
ArrayList<String> urls = new ArrayList<>();
Expand All @@ -60,19 +59,13 @@ private List<String> getRepositoriesUrls(List<Repository> repositories) {

public void execute() throws MojoExecutionException {

List<File> sources = new ArrayList<>();

if (!skipSources) {
sources.addAll(sourceDirectories);
} else {
getLog().warn("format.skipSources set, ignoring main directories");
List<File> sources;
try {
sources = getSources();
} catch (IOException exception) {
throw new MojoExecutionException("Couldn't determine canonical sources", exception);
}

if (!skipTestSources) {
sources.addAll(testSourceDirectories);
} else {
getLog().warn("format.skipTestSources set, ignoring validateOnly directories");
}
if (!sources.isEmpty()) {
try {

Expand All @@ -84,7 +77,9 @@ public void execute() throws MojoExecutionException {
showReformattedOnly,
branch,
project.getBasedir(),
useSpecifiedRepositories ? getRepositoriesUrls(mavenRepositories) : new ArrayList<String>()
useSpecifiedRepositories ?
getRepositoriesUrls(project.getRepositories()) :
new ArrayList<String>()
).format(sources);
getLog().info(result.toString());
if (validateOnly && result.unformattedFiles() != 0) {
Expand All @@ -98,4 +93,23 @@ public void execute() throws MojoExecutionException {
getLog().warn("No sources specified, skipping formatting");
}
}

private List<File> getSources() throws IOException {
SourcesBuilder builder = new SourcesBuilder(project);

if (skipSources) {
getLog().warn("format.skipSources set, ignoring main directories");
} else {
builder.addMain(sourceDirectories);
}

if (skipTestSources) {
getLog().warn("format.skipTestSources set, ignoring validateOnly directories");
} else {
builder.addTest(testSourceDirectories);
}

return builder.result();
}

}
53 changes: 53 additions & 0 deletions src/main/scala/org/antipathy/mvn_scalafmt/SourcesBuilder.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
package org.antipathy.mvn_scalafmt
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

compared to #149: moved the bulk of the logic from FormatMojo.getSources to this source, along with unit tests.


import java.io.File
import java.util.{List => JList}

import scala.jdk.CollectionConverters._

import org.apache.maven.project.MavenProject

/** Class for building a set of source dirs in the maven project */
class SourcesBuilder(project: MavenProject) {

private val sources = Set.newBuilder[File]
private val build = project.getBuild()
private val outpath = getCanonicalFile(build.getDirectory()).toPath()

private[mvn_scalafmt] def resultSet(): Set[File] =
try sources.result()
finally sources.clear()

def result(): JList[File] = resultSet().toList.asJava

def addMain(dirs: JList[File]): Unit =
if (!add(dirs)) {
appendPrimary(build.getSourceDirectory())
appendAlternative(project.getCompileSourceRoots())
}

def addTest(dirs: JList[File]): Unit =
if (!add(dirs)) {
appendPrimary(build.getTestSourceDirectory())
appendAlternative(project.getTestCompileSourceRoots())
}

private def add(dirs: JList[File]): Boolean = {
val ok = dirs != null && !dirs.isEmpty
if (ok) sources ++= dirs.asScala.map(getCanonicalFile)
ok
}

private def getCanonicalFile(dir: String): File =
project.getBasedir().toPath.resolve(dir).toFile().getCanonicalFile()

private def getCanonicalFile(dir: File): File =
project.getBasedir().toPath.resolve(dir.toPath).toFile().getCanonicalFile()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed to Path.resolve because of codacy. this replaces the code that failed in #149 because of unexpected behaviour of new File(a, b).


private def appendPrimary(dir: String): Unit =
sources += getCanonicalFile(dir + "/../scala")

private def appendAlternative(altDirs: JList[String]): Unit =
sources ++= altDirs.asScala.map(getCanonicalFile).filter(!_.toPath().startsWith(outpath))

}
120 changes: 120 additions & 0 deletions src/test/scala/org/antipathy/mvn_scalafmt/SourcesBuilderSpec.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
package org.antipathy.mvn_scalafmt

import java.io.File
import java.nio.file.{Path, Paths}
import java.util.{List => JList}

import scala.jdk.CollectionConverters._
import scala.language.implicitConversions

import org.antipathy.mvn_scalafmt.SourcesBuilderSpec._
import org.apache.maven.model.{Build, Model}
import org.apache.maven.project.MavenProject
import org.scalatest.flatspec.AnyFlatSpec
import org.scalatest.matchers.should.Matchers

class SourcesBuilderSpec extends AnyFlatSpec with Matchers {

behavior of "ProjectSourceCollectionBuilder"

it should "add nonempty main" in {
val builder = new SourcesBuilder(getMavenProject())
builder.addMain(Seq("foo", "/bar", "foo"))
builder.resultSet() shouldBe Set[File]("/xyz/foo", "/bar")
builder.resultSet() shouldBe empty
}

it should "add empty main" in {
val mavenBuild = new Build() {
setSourceDirectory("/foo/src/main/java")
}

val builder = new SourcesBuilder(getMavenProject(mavenBuild))
builder.addMain(null)
builder.resultSet() shouldBe Set[File]("/foo/src/main/scala")
builder.resultSet() shouldBe empty
}

it should "add nonempty test" in {
val builder = new SourcesBuilder(getMavenProject())
builder.addTest(Seq("foo", "/bar", "/bar"))
builder.resultSet() shouldBe Set[File]("/xyz/foo", "/bar")
builder.resultSet() shouldBe empty
}

it should "add empty test" in {
val mavenBuild = new Build() {
setTestSourceDirectory("/foo/src/test/scala")
}

val builder = new SourcesBuilder(getMavenProject(mavenBuild))
builder.addTest(null)
builder.resultSet() shouldBe Set[File]("/foo/src/test/scala")
builder.resultSet() shouldBe empty
}

it should "add nonempty both" in {
val builder = new SourcesBuilder(getMavenProject())
builder.addMain(Seq("foo", "/bar"))
builder.addTest(Seq("foo", "/qux"))
builder.resultSet() shouldBe Set[File]("/xyz/foo", "/bar", "/qux")
builder.resultSet() shouldBe empty
}

it should "add empty both" in {
val mavenBuild = new Build() {
setSourceDirectory("/foo/src/main/java")
setTestSourceDirectory("/foo/src/test/java")
}

val builder = new SourcesBuilder(getMavenProject(mavenBuild))
builder.addMain(null)
builder.addTest(null)
builder.resultSet() shouldBe Set[File]("/foo/src/main/scala", "/foo/src/test/scala")
builder.resultSet() shouldBe empty
}

it should "add empty both, filtering" in {
val mavenBuild = new Build() {
setSourceDirectory("/foo/src/main/java")
setTestSourceDirectory("/foo/src/test/java")
}
val project = getMavenProject(mavenBuild)
project.addCompileSourceRoot("/foo/src/main/scala")
project.addCompileSourceRoot("/out/src/main/scala")
project.addCompileSourceRoot("/out2/src/main/scala")
project.addTestCompileSourceRoot("/foo/src/test/scala")
project.addTestCompileSourceRoot("/out/src/test/scala")
project.addTestCompileSourceRoot("/out2/src/test/scala")

val builder = new SourcesBuilder(project)
builder.addMain(null)
builder.addTest(null)
builder.resultSet() shouldBe Set[File](
"/foo/src/main/scala",
"/out2/src/main/scala",
"/foo/src/test/scala",
"/out2/src/test/scala"
)
builder.resultSet() shouldBe empty
}

}

private object SourcesBuilderSpec {

def getMavenProject(mavenBuild: Build = new Build()) = {
mavenBuild.setDirectory("/out")
new MavenProject(new Model { setBuild(mavenBuild) }) {
setFile(new File("/xyz/pom.xml")) // sets basedir
}
}

implicit def implicitStringToPath(file: String): Path = Paths.get(file)

implicit def implicitStringToFile(file: String): File = implicitStringToPath(file).toFile

implicit def implicitStringsToFiles(files: Seq[String]): JList[File] =
files.map(implicitStringToFile).asJava

}