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

Scalafmt support #308

Merged
merged 9 commits into from
May 6, 2018
Merged
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
22 changes: 22 additions & 0 deletions docs/pages/2 - Configuring Mill.md
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,28 @@ You can use Scala compiler plugins by setting `scalacPluginIvyDeps`. The above
example also adds the plugin to `compileIvyDeps`, since that plugin's artifact
is needed on the compilation classpath (though not at runtime).

## Reformatting your code

Mill supports code formatting via [scalafmt](https://scalameta.org/scalafmt/) out of the box.

To have a formatting per-module you need to make your module extend `mill.scalalib.scalafmt.ScalafmtModule`:

```scala
// build.sc
import mill._
import mill.scalalib._
import mill.scalalib.scalafmt._

object foo extends ScalaModule with ScalafmtModule {
def scalaVersion = "2.12.4"
}
```

Now you can reformat code with `mill foo.reformat` command.

You can also reformat your project's code globally with `mill mill.scalalib.scalafmt.ScalafmtModule/reformatAll __.sources` command.
It will reformat all sources that matches `__.sources` query.

## Common Configuration

```scala
Expand Down
58 changes: 58 additions & 0 deletions scalalib/src/mill/scalalib/scalafmt/ScalafmtModule.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
package mill.scalalib.scalafmt

import ammonite.ops.{exists, ls, pwd}
import mill._
import mill.define._
import mill.scalalib._

trait ScalafmtModule extends JavaModule {

def reformat(): Command[Unit] = T.command {
ScalafmtWorkerModule
.worker()
.reformat(
filesToFormat(sources()),
scalafmtConfig().head,
scalafmtDeps().map(_.path)
)
}

def scalafmtVersion: T[String] = "1.5.1"

def scalafmtConfig: Sources = T.sources(pwd / ".scalafmt.conf")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Don't we have an alternative for Sources for a single file?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think so; we could add one if it comes up repeatedly, but for now just picking the first item in the Agg[PathRef] may be fine


def scalafmtDeps: T[Agg[PathRef]] = T {
Lib.resolveDependencies(
ScalaWorkerModule.repositories,
Lib.depToDependency(_, "2.12.4"),
Seq(ivy"com.geirsson::scalafmt-cli:${scalafmtVersion()}")
)
}

protected def filesToFormat(sources: Seq[PathRef]) = {
for {
pathRef <- sources if exists(pathRef.path)
file <- ls.rec(pathRef.path) if file.isFile && file.ext == "scala"
} yield PathRef(file)
}

}

object ScalafmtModule extends ExternalModule with ScalafmtModule {

def reformatAll(sources: mill.main.Tasks[Seq[PathRef]]): Command[Unit] =
T.command {
val files = Task.sequence(sources.value)().flatMap(filesToFormat)
ScalafmtWorkerModule
.worker()
.reformat(
files,
scalafmtConfig().head,
scalafmtDeps().map(_.path)
)
}

implicit def millScoptTargetReads[T] = new mill.main.Tasks.Scopt[T]()

lazy val millDiscover = Discover[this.type]
}
58 changes: 58 additions & 0 deletions scalalib/src/mill/scalalib/scalafmt/ScalafmtWorker.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
package mill.scalalib.scalafmt

import ammonite.ops.{Path, exists}
import mill._
import mill.define.{Discover, ExternalModule, Worker}
import mill.modules.Jvm
import mill.util.Ctx

import scala.collection.mutable

object ScalafmtWorkerModule extends ExternalModule {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It wasn't immediately clear that worker must live in a separate module to be a single one in a build.
Also, I copy-pasted extends ExternalModule from scala and scalajs workers. Why should it be an ExternalModule, not just a Module ?
When I tried extends Module complilation failed with:

[error] /Users/rockjam/projects/mill/scalalib/src/mill/scalalib/scalafmt/ScalafmtWorker.scala:11:54: Modules, Targets and Commands can only be defined within a mill Module
[error] private[scalafmt] object ScalafmtWorker extends mill.Module {

Copy link
Member

Choose a reason for hiding this comment

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

@rockjam being an ExternalModule allows a task myTask to be run via mill.scalalib.scalafmt.ScalafmtWorkerModule/myTask. Normal Module objects must be defined in the build.sc file and be run via their object selector from the root of build.sc.

If you want to let people run ScalaFmt without needing to extend anything, you could make a task in the ExternalModule that lets people run e.g. mill.scalalib.scalafmt.ScalafmtWorkerModule/fmt __.sources to format the sources of all their ScalaModules. See how the mill.scalalib.PublishModule/publishAll command works as an example.

As for being separate Mill submodules, we only needed to put things like ScalaWorker/ScalaJSWorker in there because we needed code that runs with some heavyweight dependencies on the classpath that might collide (e.g. different Scala.js versions): the dependencies are isolated within a classloader and interacted with via reflection. If you don't have any heavyweight dependencies, or you do but you can just interact with them directly via reflection/subprocesses, then there's no need to isolate them in a separate Mill build module

def worker: Worker[ScalafmtWorker] = T.worker { new ScalafmtWorker() }

lazy val millDiscover = Discover[this.type]
}

private[scalafmt] class ScalafmtWorker {
private val reformatted: mutable.Map[Path, Int] = mutable.Map.empty
private var configSig: Int = 0

def reformat(input: Seq[PathRef],
scalafmtConfig: PathRef,
scalafmtClasspath: Agg[Path])(implicit ctx: Ctx): Unit = {
val toFormat =
if (scalafmtConfig.sig != configSig) input
else
input.filterNot(ref => reformatted.get(ref.path).contains(ref.sig))

if (toFormat.nonEmpty) {
ctx.log.info(s"Formatting ${toFormat.size} Scala sources")
reformatAction(toFormat.map(_.path),
scalafmtConfig.path,
scalafmtClasspath)
reformatted ++= toFormat.map { ref =>
val updRef = PathRef(ref.path)
updRef.path -> updRef.sig
}
configSig = scalafmtConfig.sig
} else {
ctx.log.info(s"Everything is formatted already")
}
}

private val cliFlags = Seq("--non-interactive", "--quiet")

private def reformatAction(toFormat: Seq[Path],
config: Path,
classpath: Agg[Path])(implicit ctx: Ctx) = {
val configFlags =
if (exists(config)) Seq("--config", config.toString) else Seq.empty
Jvm.subprocess(
"org.scalafmt.cli.Cli",
classpath,
mainArgs = toFormat.map(_.toString) ++ configFlags ++ cliFlags
)
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
foo.bar = 2
7 changes: 7 additions & 0 deletions scalalib/test/resources/scalafmt/core/src/Main.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@

object Main extends App{
val person = Person.fromString("rockjam:25")
val greeting = s"hello ${person.name}, your age is: ${person.age}"
println(greeting)
}

12 changes: 12 additions & 0 deletions scalalib/test/resources/scalafmt/core/src/Person.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
object Person {
def fromString(s: String): Person = {
val Array(name, age) = s.split(":")
Person(
name,
age.toInt)
}
}



case class Person(name: String, age: Int)
105 changes: 105 additions & 0 deletions scalalib/test/src/mill/scalalib/scalafmt/ScalafmtTests.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
package mill.scalalib.scalafmt

import ammonite.ops._
import mill.main.Tasks
import mill.scalalib.ScalaModule
import mill.util.{TestEvaluator, TestUtil}
import utest._
import utest.framework.TestPath

object ScalafmtTests extends TestSuite {

trait TestBase extends TestUtil.BaseModule {
def millSourcePath =
TestUtil.getSrcPathBase() / millOuterCtx.enclosing.split('.')
}

object ScalafmtTestModule extends TestBase {
object core extends ScalaModule with ScalafmtModule {
def scalaVersion = "2.12.4"
}
}

val resourcePath = pwd / 'scalalib / 'test / 'resources / 'scalafmt

def workspaceTest[T, M <: TestUtil.BaseModule](
m: M,
resourcePath: Path = resourcePath)(t: TestEvaluator[M] => T)(
implicit tp: TestPath): T = {
val eval = new TestEvaluator(m)
rm(m.millSourcePath)
rm(eval.outPath)
mkdir(m.millSourcePath / up)
cp(resourcePath, m.millSourcePath)
t(eval)
}

def tests: Tests = Tests {
'scalafmt - {
def checkReformat(reformatCommand: mill.define.Command[Unit]) =
workspaceTest(ScalafmtTestModule) { eval =>
val before = getProjectFiles(ScalafmtTestModule.core, eval)

// first reformat
val Right(_) = eval.apply(reformatCommand)

val firstReformat = getProjectFiles(ScalafmtTestModule.core, eval)

assert(
firstReformat("Main.scala").modifyTime > before("Main.scala").modifyTime,
firstReformat("Main.scala").content != before("Main.scala").content,
firstReformat("Person.scala").modifyTime > before("Person.scala").modifyTime,
firstReformat("Person.scala").content != before("Person.scala").content,
// resources files aren't modified
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wanted to separate different parts of the test with nested test cases(like 'firstTime, 'cached, 'afterChange), but it didn't work, because I needed shared test evaluator, and if I did nested tests - test evaluator was created every time for each test. I couldn't create TestEvaluator outside of the Tests block either because it requires implicit TestPath, that is not available outside of the Tests block.
So, is it possible to do this with utest?

Copy link
Member

Choose a reason for hiding this comment

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

It's not easy to do with uTest, leaving them in the same test seems ok for now

firstReformat("application.conf").modifyTime == before(
"application.conf").modifyTime
)

// cached reformat
val Right(_) = eval.apply(reformatCommand)

val cached = getProjectFiles(ScalafmtTestModule.core, eval)

assert(
cached("Main.scala").modifyTime == firstReformat("Main.scala").modifyTime,
cached("Person.scala").modifyTime == firstReformat("Person.scala").modifyTime,
cached("application.conf").modifyTime == firstReformat(
"application.conf").modifyTime
)

// reformat after change
write.over(cached("Main.scala").path,
cached("Main.scala").content + "\n object Foo")

val Right(_) = eval.apply(reformatCommand)

val afterChange = getProjectFiles(ScalafmtTestModule.core, eval)

assert(
afterChange("Main.scala").modifyTime > cached("Main.scala").modifyTime,
afterChange("Person.scala").modifyTime == cached("Person.scala").modifyTime,
afterChange("application.conf").modifyTime == cached(
"application.conf").modifyTime
)
}

'reformat - checkReformat(ScalafmtTestModule.core.reformat())
'reformatAll - checkReformat(
ScalafmtModule.reformatAll(Tasks(Seq(ScalafmtTestModule.core.sources))))
}
}

case class FileInfo(content: String, modifyTime: Long, path: Path)

def getProjectFiles(m: ScalaModule, eval: TestEvaluator[_]) = {
val Right((sources, _)) = eval.apply(m.sources)
val Right((resources, _)) = eval.apply(m.resources)

val sourcesFiles = sources.flatMap(p => ls.rec(p.path))
val resourcesFiles = resources.flatMap(p => ls.rec(p.path))
(sourcesFiles ++ resourcesFiles).map { p =>
p.name -> FileInfo(read(p), p.mtime.toMillis, p)
}.toMap
}

}