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

Scalafmt support #308

merged 9 commits into from
May 6, 2018

Conversation

rockjam
Copy link
Collaborator

@rockjam rockjam commented May 1, 2018

This PR introduces the module that provides scalafmt support. 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

@rockjam rockjam changed the title Scalafmt "plugin" Scalafmt support May 1, 2018

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 scalafmtConfig: Sources = T.sources(pwd / ".scalafmt.conf")

def scalafmtDeps = resolveDeps(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should we use certain scala version to resolve deps to stay stable no matter what version of scala mill uses?

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 have any opinion, though I suppose scalafmt is meant to do the same thing regardless of scalaVersion?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@rockjam rockjam left a comment

Choose a reason for hiding this comment

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

@lihaoyi
Copy link
Member

lihaoyi commented May 1, 2018

looks good to me i guess

@rockjam
Copy link
Collaborator Author

rockjam commented May 1, 2018

Also, do we keep scalafmt support in scalalib, or should I move it to some other subproject?

@Ammonite-Bot
Copy link
Collaborator

Ammonite-Bot commented May 1, 2018 via email


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

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

@rockjam
Copy link
Collaborator Author

rockjam commented May 5, 2018

@lihaoyi added some tests, made external module for reformatting all the sources and added some lines into documentation. Ready for review

Copy link
Member

@lihaoyi lihaoyi left a comment

Choose a reason for hiding this comment

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

looks good to me

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
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

@rockjam rockjam merged commit f7a02a4 into com-lihaoyi:master May 6, 2018
@lefou lefou added this to the 0.2.1 milestone May 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants