Scalafmt support #308
Scalafmt support #308
Conversation
|
|
||
| def scalafmtVersion: T[String] = "1.5.1" | ||
|
|
||
| def scalafmtConfig: Sources = T.sources(pwd / ".scalafmt.conf") |
There was a problem hiding this comment.
Don't we have an alternative for Sources for a single file?
There was a problem hiding this comment.
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 scalafmtConfig: Sources = T.sources(pwd / ".scalafmt.conf") | ||
|
|
||
| def scalafmtDeps = resolveDeps( |
There was a problem hiding this comment.
Should we use certain scala version to resolve deps to stay stable no matter what version of scala mill uses?
There was a problem hiding this comment.
I don't have any opinion, though I suppose scalafmt is meant to do the same thing regardless of scalaVersion?
There was a problem hiding this comment.
Yes, version of scala doesn't affect the result of formatting with scalafmt. What I meant is the situation like this:
At some point, scala 2.13 is released. We decide to migrate mill to 2.13, but we won't be able to do that until scalafmt for 2.13 is released.
|
looks good to me i guess |
|
Also, do we keep scalafmt support in scalalib, or should I move it to some other subproject? |
|
I think leaving it in the main scalalib for now is ok; if we’re properly
isolating the scalafmt dependencies in a new classloader or subprocess,
there isn’t much cost or risk of classpath conflicts
…On Tue, 1 May 2018 at 3:29 AM, Nikolay Tatarinov ***@***.***> wrote:
Also, do we keep scalafmt support in scalalib, or should I move it to some
other subproject?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#308 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ATpwjFD2TsfrmBsHLVY9kd-MUpRtmFGYks5tuDkZgaJpZM4TtZsc>
.
|
…tence; add quiet flags to scalafmt CLI
|
|
||
| import scala.collection.mutable | ||
|
|
||
| object ScalafmtWorkerModule extends ExternalModule { |
There was a problem hiding this comment.
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 {
There was a problem hiding this comment.
@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
| 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
It's not easy to do with uTest, leaving them in the same test seems ok for now
|
@lihaoyi added some tests, made external module for reformatting all the sources and added some lines into documentation. Ready for review |
| 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 |
There was a problem hiding this comment.
It's not easy to do with uTest, leaving them in the same test seems ok for now
This PR introduces the module that provides
scalafmtsupport. It features stateful worker that caches file signatures internally to avoid formatting sources that were already formatted. Caching is also aware of changes in.scalafmt.conf, in this case we reformat everything from scratch.It would be great if we could discuss this PR to figure out the "idiomatic" way to write such mutable tasks/commands that change sources, but don't provide any valuable result apart from side-effect