com-lihaoyi
Format using scalafmt command rather than ScalafmtModule/checkFormatAll task
Don't know what you think of this. This formats all sources by running a blind scalafmt at the root of Mill, rather than going through Mill with ./mill mill.scalalib.scalafmt.ScalafmtModule/checkFormatAll __.sources.
Blindly running scalafmt at the root of a project is a common thing to do. This PR makes sure it works fine here.
This formats things like examples and resources in particular.
I think it's reasonable, and arguably better than the fine grained approach of doing it vis __.sources, but we need to make sure it works and thete's escape hatches to disable when it doesn't (e.g. some kind of .fmtignore file?)
e.g. in this case it mangles the comments we parse as part of our example test suite. We either need to change the parser or change the formatter or add some kind of ignore config to make it work
In principle I think a coarse grained formatting has many advantages over the fine grained approach of going through the build tool to list sources. In practice I think it still can be done, just needs to be done carefully so we can handle the inevitable edge cases (template files, magic comments, generated code, output folders, test data, ...)
Maybe something we can do eventually but probably not an immediate priority for the 0.12.0/0.13.0 releases coming up
Converted to draft, so that we don't merge by mistake before https://github.com/com-lihaoyi/mill/pull/3526
Can't we add a scalafmt command to the external ScalaFmtModule which accepts free arguments and runs from the workspace root as an drop-in replacement for scalafmt cli? Convenient and we could avoid the extra ci/scalafmt.sh script.
@lefou I did that for https://github.com/com-lihaoyi/mill/pull/3511#discussion_r1752975390 (and I personally use the native scalafmt binaries that the script pulls), but we can add both, maybe. The native scalafmt is quite convenient, as it's much faster (more for local use than on the CI, actually).
@lefou I did that for #3511 (comment) (and I personally use the native scalafmt binaries that the script pulls), but we can add both, maybe. The native scalafmt is quite convenient, as it's much faster (more for local use than on the CI, actually).
I fail to see that you did that, but it's probably squashed away (which is btw not so nice for reviewing PRs).
I thought, Haoyi was commenting on the coursier/setup-action pulling scalafmt by using coursier which is essentially one of many ways to install another binary on the machine. What I'm proposing is, that we use Mill to act as an drop-in replacement for scalafmt in case the user wants it. Mill already has the classpath anyways, so it's rather easy to forward all params to the right main method. From a user perspective, it does not require a new locally installed tool beside the already configured Mill build.
It may not be as instant as the scalafmt cli, but still faster than what the existing commands provide.
Yes I don't want an additional dependency on the coursier install step. We currently bootstrap Mill pretty narrowly using only an installed JVM, and I would like to keep it that way.
This discussion also needs to broadened into what kind of autoformatting workflows we recommend to Mill users. As of now in master, running ScalaFmt is done via:
./mill mill.scalalib.ScalafmtModule/
Unless there's some requirements unique to Mill's own codebase, whatever autoformatting workflow we do ourselves should also be the best practice we recommend to users. I don't think Mill is that special w.r.t. autoformatting, so we should try to keep the dev/user workflows the same here. And users should not need to install separate binaries in order to use Mill
That makes sense. I guess a command to blindly reformat the workspace could be added in ScalafmtModule .