Format using scalafmt command rather than ScalafmtModule/checkFormatAll task

[alexarchambault]

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.

[lihaoyi]

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

[lihaoyi]

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

[lihaoyi]

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, ...)

[lihaoyi]

Maybe something we can do eventually but probably not an immediate priority for the 0.12.0/0.13.0 releases coming up

[lefou]

I'd expect some failing tests. Those, that test the scalafmt integration.

[lihaoyi]

I'd like to avoid adding a dependency on a global scalafmt binary. Adding it in CI will necessitate people installing it locally to match the CI behavior, doubles the number of things that need to be installed (previously it was only 1: the JVM).

We already have Scalafmt on the Mill classpath, so instead of installing it globally we could define a top-level command in build.mill that uses Scalafmt programmatically (either inprocess or subprocess) on the files in T.workspace. That would achieve the same thing but maintain Mill's minimal unmanaged dependency footprint

[lihaoyi]

Alternatively, if we want to bypass mill entirely (e.g. for performance), we could have a ci/scalafmt.sh script to download the ScalaFmt graal native executable appropriate for your OS, cache it somewhere, and run that

[alexarchambault]

That's what I did in the last push

[lihaoyi]

The only reason we don't need to ignore the rest of example/ is because Scalafmt doesn't pick up the .mill files. If Scalafmt learns how to do so, or if we rename them to .mill.scala, we'll need to go back and ignore the rest of example/ as well

[lefou]

Would be nice to split the tooling changes from the actual formatting commit, so we can later add it to .git-blame-ignore-revs

[alexarchambault]

Converted to draft, so that we don't merge by mistake before #3526

[lefou]

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.

[alexarchambault]

@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).

[lefou]

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

[lihaoyi]

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

[alexarchambault]

That makes sense. I guess a command to blindly reformat the workspace could be added in ScalafmtModule .