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.

Sign up for GitHub

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

Jump to bottom

New scalafmt sbt plugin #1085

Merged
merged 9 commits into from
Dec 12, 2017
Merged

New scalafmt sbt plugin #1085

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d9d0920
WIP new scalafmt sbt plugin
vovapolu Nov 26, 2017
e1812be
First tests for sbt plugin
vovapolu Nov 29, 2017
1ef7273
More tests for sbt-plugin
vovapolu Nov 30, 2017
e33449b
Fixes
vovapolu Dec 1, 2017
3ef624e
moved megaCache clearing
vovapolu Dec 1, 2017
5bff421
Fixes 1.5 (without caching)
vovapolu Dec 3, 2017
7b52844
taskDyn to settingDyn
vovapolu Dec 3, 2017
bed8e83
Fixes 1.55
vovapolu Dec 12, 2017
c0e049b
Minor fixes
vovapolu Dec 12, 2017
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
2 changes: 1 addition & 1 deletion scalafmt-sbt-tests/src/sbt-test/scalafmt-sbt/sbt/.scalafmt6.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
style = defaultWithAlign
style = default
30 changes: 29 additions & 1 deletion scalafmt-sbt-tests/src/sbt-test/scalafmt-sbt/sbt/build.sbt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,11 @@ lazy val p5 = project.settings(
)
lazy val p6 = project.settings(
scalaVersion := "2.12.1",
scalafmtConfig := file(".scalafmt6.conf")
scalafmtConfig := Some(file(".scalafmt6.conf"))
)
lazy val p7 = project.settings(
scalaVersion := "2.12.1",
scalafmtConfig := None
)

def assertContentsEqual(file: File, expected: String): Unit = {
Expand Down Expand Up @@ -69,6 +73,8 @@ TaskKey[Unit]("check") := {
expectedMainTest
)
}


assertContentsEqual(
file(s"p5/src/main/scala/Test.scala"),
"""
Expand All @@ -87,6 +93,8 @@ TaskKey[Unit]("check") := {
|}
""".stripMargin
)


assertContentsEqual(
file(s"p6/src/main/scala/Test.scala"),
"""
Expand All @@ -106,6 +114,26 @@ TaskKey[Unit]("check") := {
""".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"),
"""
Expand Down
1 change: 1 addition & 0 deletions scalafmt-sbt-tests/src/sbt-test/scalafmt-sbt/sbt/p7/src
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
../p3/src
7 changes: 7 additions & 0 deletions scalafmt-sbt-tests/src/sbt-test/scalafmt-sbt/sbt/test
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
-> 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
Expand All @@ -21,4 +23,9 @@
> p6/compile:scalafmtCheck
> p6/test:scalafmtCheck

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

> check
55 changes: 37 additions & 18 deletions scalafmt-sbt/src/main/scala/org/scalafmt/sbt/ScalafmtPlugin.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,23 +15,32 @@ object ScalafmtPlugin extends AutoPlugin {
override def trigger: PluginTrigger = allRequirements

object autoImport {
val scalafmt = taskKey[Unit]("Format Scala sources")
val scalafmt = taskKey[Unit]("Format Scala sources.")
val scalafmtCheck =
taskKey[Boolean]("Check that Scala sources is formatted properly")
taskKey[Boolean](
"Fails if a Scala source is mis-formatted. Does not write to files.")
val scalafmtOnCompile =
settingKey[Boolean]("Format source when compiling")
val scalafmtConfig = taskKey[File]("Scalafmt config file")
val scalafmtSbt = taskKey[Unit]("Format SBT sources")
settingKey[Boolean](
"Format Scala source files on compile, off by default.")
val scalafmtConfig = taskKey[Option[File]](
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean that you must scalafmtConfig := Some(file(".scalafmt.conf")) to avoid the default settings? I would prefer to make this non-Option and if the file does not exist then use default settings.

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 searches for .scalafmt.conf file by default. And you can redefine it for your custom 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 see, cool. OK this is fine then 👍

"Optional location of .scalafmt.conf file. If None the default config is used.")
val scalafmtSbt = taskKey[Unit](
"Format *.sbt and project/*.scala files for this sbt build.")
val scalafmtSbtCheck =
taskKey[Boolean]("Check that SBT sources is formatted properly")
val scalafmtOnly = inputKey[Unit]("Format only given file")
taskKey[Boolean](
"Fails if a *.sbt or project/*.scala source is mis-formatted. Does not write to files.")
val scalafmtOnly = inputKey[Unit]("Format only given file.")
}
import autoImport._

private val scalaConfig = scalafmtConfig.map(Config.fromHoconFile(_) match {
case Configured.Ok(conf) => conf
case Configured.NotOk(e) => throw new MessageOnlyException(e.msg)
})
private val scalafmtDoFormatOnCompile =
taskKey[Unit]("Format Scala source files if scalafmtOnCompile is on.")

private val scalaConfig =
scalafmtConfig.map(_.map(Config.fromHoconFile(_) match {
Copy link
Member

Choose a reason for hiding this comment

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

Could we at least cache this by the timestamp of the file? I feel it's unnecessary to reparse the config for every project/config on every compile.

Copy link
Member

Choose a reason for hiding this comment

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

Here's what the IntelliJ plugin uses https://github.com/scalameta/scalafmt/blob/0f391aa351c59432d1cf6693b5658ab81d4de517/scalafmt-cli/src/main/scala/org/scalafmt/cli/StyleCache.scala

Copy link
Member

Choose a reason for hiding this comment

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

I see, sure we can leave it for a separate PR :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here's what the IntelliJ plugin uses

@olafurpg maybe simply reuse that object? It does just what we need

Copy link
Member

Choose a reason for hiding this comment

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

Go ahead, we might want to move it to the core module

case Configured.Ok(conf) => conf
case Configured.NotOk(e) => throw new MessageOnlyException(e.msg)
}).getOrElse(ScalafmtConfig.default))
private val sbtConfig = scalaConfig.map(_.forSbt)

private type Input = String
Copy link
Member

Choose a reason for hiding this comment

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

This does not help readability IMO, either create a value class or use String.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@dwijnand Why did you put a dislike here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just because I disagree (even if the type Input is just an alias, I think it's can be useful to have such aliases).

Copy link
Member

Choose a reason for hiding this comment

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

My view is that you might as well make it a value class then, a type alias like this gives a false sense of type safety

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

well, it's 2 vs 2, so I leave it :)

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough, I don't feel strongly about this. I'm guilty of doing this myself sometimes 😏

Expand Down Expand Up @@ -137,13 +146,16 @@ object ScalafmtPlugin extends AutoPlugin {
checkSources(sbtSources.value, sbtConfig.value, streams.value.log)
checkSources(projectSources.value, scalaConfig.value, streams.value.log)
},
compileInputs in compile := Def.taskDyn {
val task =
if (scalafmtOnCompile.value) scalafmt in resolvedScoped.value.scope
else Def.task(())
val previousInputs = (compileInputs in compile).value
task.map(_ => previousInputs)
scalafmtDoFormatOnCompile := Def.taskDyn {
Copy link
Member

Choose a reason for hiding this comment

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

As recommended by @sjrd we should make this a settingDyn

if (scalafmtOnCompile.value) {
scalafmt in resolvedScoped.value.scope
Copy link
Member

Choose a reason for hiding this comment

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

This will run scalafmt on all sources on every compile, including nop compiles. It does not take advantage of incrementality. Is this expected/intended behavior? cc/ @fommil I think scalariform uses FileFunction.cached to make this incremental https://github.com/sbt/sbt-scalariform/blob/a378b724a76ee94819939efc0e9adebd896e57d9/src/main/scala/com/typesafe/sbt/Scalariform.scala#L85

Copy link

@fommil fommil Dec 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

yeah it's only supposed to be formatting files that are being compiled, not all of them

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let's leave caching for next PRs. I would like to implement it entirely, not only in some specific places. I will write a warning in the task description for now :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@olafurpg that's a wrong solution. I don't know why scalariform uses it. FileFunction.cached accepts set of files, if you modify them (and we modify them by formatting), then it thinks that files are changed and runs the function again. So it needs two iteration to "converge" to formatted source and forget about it.

} else {
Def.task(())
}
}.value,
compileInputs in compile := (compileInputs in compile)
.dependsOn(scalafmtDoFormatOnCompile)
.value,
scalafmtOnly := {
val files = spaceDelimited("<files>").parsed
val absFiles = files.flatMap(fileS => {
Expand All @@ -166,7 +178,14 @@ object ScalafmtPlugin extends AutoPlugin {
Seq(Compile, Test).flatMap(inConfig(_)(scalafmtConfigSettings))

override def buildSettings: Seq[Def.Setting[_]] = Seq(
scalafmtConfig := (baseDirectory in ThisBuild).value / ".scalafmt.conf"
scalafmtConfig := {
val path = (baseDirectory in ThisBuild).value / ".scalafmt.conf"
if (path.exists()) {
Some(path)
} else {
None
}
}
)

override def globalSettings: Seq[Def.Setting[_]] = Seq(
Expand Down