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

New scalafmt sbt plugin #1085

Merged
merged 9 commits into from
Dec 12, 2017
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
17 changes: 14 additions & 3 deletions build.sbt
Original file line number Diff line number Diff line change
Expand Up @@ -66,13 +66,13 @@ def isOnly(scalaV: String) = Seq(
crossScalaVersions := Seq(scalaV)
)

lazy val `scalafmt-sbt` = project
lazy val `scalafmt-cli-sbt` = project
.configs(IntegrationTest)
.settings(
allSettings,
Defaults.itSettings,
mimaPreviousArtifacts := Set.empty,
moduleName := "sbt-scalafmt",
moduleName := "sbt-cli-scalafmt",
isOnly(scala212),
sbtPlugin := true,
sbtVersion in Global := "1.0.0",
Expand All @@ -87,6 +87,17 @@ lazy val `scalafmt-sbt` = project
)
.dependsOn(cli)

lazy val `scalafmt-sbt` = project
.settings(
allSettings,
mimaPreviousArtifacts := Set.empty,
moduleName := "sbt-scalafmt",
isOnly(scala212),
sbtPlugin := true,
sbtVersion in Global := "1.0.0"
Copy link

Choose a reason for hiding this comment

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

Don't put settings in Global in the settings of a project. It is extremely confusing as that affects the whole build, not just the project. It makes no sense.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

old plugin has this setting too :)

Copy link

Choose a reason for hiding this comment

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

Then fix the old plugin too ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

This is effectively defining which sbt version the plugin targets, rather than defaulting whatever sbt version you're using for the whole build as defined in build.properties. I've used this in the past to ensure my plugin works on old versions of sbt while using the latest version to build the plugin.

)
.dependsOn(coreJVM)

lazy val `scalafmt-sbt-tests` = project
.settings(
allSettings,
Expand Down Expand Up @@ -333,7 +344,7 @@ def ciCommands = Seq(
Nil
),
Command.command("ci-sbt-scalafmt") { s =>
"scalafmt-sbt/it:test" ::
"scalafmt-cli-sbt/it:test" ::
s
},
Command.command("ci-publish") { s =>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
package org.scalafmt.sbt
import scala.meta.internal.tokenizers.PlatformTokenizerCache
import sbt._
import Keys._

object ScalafmtPlugin extends AutoPlugin {
override def trigger: PluginTrigger = allRequirements
object autoImport {
val scalafmt: Command =
Command.args("scalafmt", "run the scalafmt command line interface.") {
case (state, args) =>
org.scalafmt.cli.Cli.main("--non-interactive" +: args.toArray)
PlatformTokenizerCache.megaCache.clear()
Copy link
Member

Choose a reason for hiding this comment

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

You want to run this in the new plugin too, megaCache will eventually go away but for now it's best to call it on every file. scalameta/scalameta#1068

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Call on every file? Your plugin calls it once at start.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And when I should call it? Even when I check source or only when format?

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 a bit trickier to decide compared to the cli, but I think we can do it once per config/project. This should ideally run whenever scalafmt is not running, otherwise it shoudl be a well-behaving cache and the only penalty for clearing it too often is a very minor slowdown hit.

state
}
}
override def globalSettings: Seq[Def.Setting[_]] =
Seq(
commands += autoImport.scalafmt
) ++
addCommandAlias("scalafmtTest", "scalafmt --test") ++
addCommandAlias("scalafmtDiffTest", "scalafmt --diff --test") ++
addCommandAlias("scalafmtDiff", "scalafmt --diff")

}
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,11 @@ object Scalafmt {
**/
def configWithDialect(
config: ScalafmtConfig,
dialect: Dialect
): ScalafmtConfig =
dialect: Dialect): ScalafmtConfig =
config.withDialect(dialect)

def configForSbt(
config: ScalafmtConfig
): ScalafmtConfig =
config.forSbt
}
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,8 @@ case class ScalafmtConfig(
def withDialect(dialect: Dialect): ScalafmtConfig =
copy(runner = runner.copy(dialect = dialect))

def forSbt: ScalafmtConfig = copy(runner = runner.forSbt)

def reformatDocstrings: Boolean = docstrings != Docstrings.preserve
def scalaDocs: Boolean = docstrings == Docstrings.ScalaDoc
lazy val alignMap: Map[String, Regex] =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ TaskKey[Unit]("check") := {
new File("project/plugins.sbt"),
"""
|addSbtPlugin(
| "com.geirsson" % "sbt-scalafmt" % System.getProperty("plugin.version")
| "com.geirsson" % "sbt-cli-scalafmt" % System.getProperty("plugin.version")
|)
""".stripMargin
)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
public class TestJava
{
public static void main(String[] args) {
System.out.println("Hello World!");
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
public class MainTestJava
{
public static void main(String[] args) {
System.out.println("Hello World!");
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
addSbtPlugin (
"com.geirsson" % "sbt-cli-scalafmt" % System.getProperty("plugin.version"))

Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
style = IntelliJ
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
style = default
project.excludeFilters = [
MainTest
]
148 changes: 148 additions & 0 deletions scalafmt-sbt-tests/src/sbt-test/scalafmt-sbt/sbt/build.sbt
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
import java.io.File

fork in ThisBuild := true

lazy val p123 = project
.in(file("."))
.aggregate(
p1,
p2,
p3
)

lazy val p1 = project.settings(
scalaVersion := "2.10.5"
)
lazy val p2 = project.settings(
scalaVersion := "2.11.8"
)
lazy val p3 = project.settings(
scalaVersion := "2.12.1"
)
lazy val p4 = project.settings(
scalaVersion := "2.12.1"
)
lazy val p5 = project.settings(
scalaVersion := "2.12.1",
scalafmtOnCompile := true
)
lazy val p6 = project.settings(
scalaVersion := "2.12.1",
scalafmtConfig := Some(file(".scalafmt6.conf"))
)
lazy val p7 = project.settings(
scalaVersion := "2.12.1",
scalafmtConfig := None
)

def assertContentsEqual(file: File, expected: String): Unit = {
val obtained =
scala.io.Source.fromFile(file).getLines().mkString("\n")

if (obtained.trim != expected.trim) {
val msg =
s"""File: $file
|Obtained output:
|$obtained
|Expected:
|$expected
|""".stripMargin
System.err.println(msg)
throw new Exception(msg)
}
}

TaskKey[Unit]("check") := {
(1 to 4).foreach { i =>
val expectedTest =
"""
|object Test {
| foo(
| a, // comment
| b
| )
|}
""".stripMargin
val expectedMainTest = expectedTest.replaceFirst("Test", "MainTest")
assertContentsEqual(
file(s"p$i/src/main/scala/Test.scala"),
expectedTest
)
assertContentsEqual(
file(s"p$i/src/test/scala/MainTest.scala"),
expectedMainTest
)
}


assertContentsEqual(
file(s"p5/src/main/scala/Test.scala"),
"""
|object Test {
| def foo(a: Int, // comment
| b: Double) = ???
|}
""".stripMargin
)
assertContentsEqual(
file(s"p5/src/test/scala/MainTest.scala"),
"""
|object MainTest {
| def foo(a: Int, // comment
| b: Double) = ???
|}
""".stripMargin
)


assertContentsEqual(
file(s"p6/src/main/scala/Test.scala"),
"""
|object Test {
| foo(a, // comment
| b)
|}
""".stripMargin
)
assertContentsEqual(
file(s"p6/src/test/scala/MainTest.scala"),
"""
|object
|MainTest
|{
| foo(a, // comment
| b)
|}
|
""".stripMargin
)


assertContentsEqual(
file(s"p7/src/main/scala/Test.scala"),
"""
|object Test {
| foo(a, // comment
| b)
|}
""".stripMargin
)
assertContentsEqual(
file(s"p7/src/test/scala/MainTest.scala"),
"""
|object MainTest {
| foo(a, // comment
| b)
|}
""".stripMargin
)

assertContentsEqual(
file("project/plugins.sbt"),
"""
|addSbtPlugin(
| "com.geirsson" % "sbt-scalafmt" % System.getProperty("plugin.version")
|)
""".stripMargin
)
}
1 change: 1 addition & 0 deletions scalafmt-sbt-tests/src/sbt-test/scalafmt-sbt/sbt/p1
1 change: 1 addition & 0 deletions scalafmt-sbt-tests/src/sbt-test/scalafmt-sbt/sbt/p2
1 change: 1 addition & 0 deletions scalafmt-sbt-tests/src/sbt-test/scalafmt-sbt/sbt/p3
4 changes: 4 additions & 0 deletions scalafmt-sbt-tests/src/sbt-test/scalafmt-sbt/sbt/p4/p4.sbt
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
lazy val settings =
Seq(
scalaVersion :=
"2.12.1")
1 change: 1 addition & 0 deletions scalafmt-sbt-tests/src/sbt-test/scalafmt-sbt/sbt/p4/src
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
object
Test
{
def foo(a: Int, // comment
b: Double) = ???
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
object
MainTest
{
def foo(a: Int, // comment
b: Double) = ???
}
1 change: 1 addition & 0 deletions scalafmt-sbt-tests/src/sbt-test/scalafmt-sbt/sbt/p6/src
1 change: 1 addition & 0 deletions scalafmt-sbt-tests/src/sbt-test/scalafmt-sbt/sbt/p7/src
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
sbt.version=1.0.0
31 changes: 31 additions & 0 deletions scalafmt-sbt-tests/src/sbt-test/scalafmt-sbt/sbt/test
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
-> p123/compile:scalafmtCheck
-> p123/test:scalafmtCheck
> p123/compile:scalafmt
Copy link
Member

Choose a reason for hiding this comment

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

Should we maybe add a negative check here?

-> p123/compile:scalafmtCheck

to validate check fails on misformatted files.

> p123/test:scalafmt
> p123/compile:scalafmtCheck
> p123/test:scalafmtCheck

> scalafmtSbt
> scalafmtSbtCheck

> p4/scalafmtOnly src/main/scala/Test.scala src/test/scala/MainTest.scala p4.sbt
> p4/compile:scalafmtCheck
> p4/test:scalafmtCheck
> p4/scalafmtSbtCheck

> p5/compile
> p5/test:compile
> p5/compile:scalafmtCheck
> p5/test:scalafmtCheck

> p6/compile:scalafmt
> p6/test:scalafmt
> p6/compile:scalafmtCheck
> p6/test:scalafmtCheck

> p7/compile:scalafmt
> p7/test:scalafmt
> p7/compile:scalafmtCheck
> p7/test:scalafmtCheck

> check
Loading