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

Format using scalafmt command rather than ScalafmtModule/checkFormatAll task #3511

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

alexarchambault
Copy link
Contributor

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.

@alexarchambault alexarchambault marked this pull request as draft September 10, 2024 12:39
@alexarchambault alexarchambault force-pushed the actually-format-all-files branch 2 times, most recently from 34bdfbf to 3bb67e4 Compare September 10, 2024 12:41
@lihaoyi
Copy link
Member

lihaoyi commented Sep 10, 2024

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
Copy link
Member

lihaoyi commented Sep 10, 2024

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
Copy link
Member

lihaoyi commented Sep 10, 2024

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
Copy link
Member

lihaoyi commented Sep 10, 2024

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

@lefou
Copy link
Member

lefou commented Sep 10, 2024

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

Comment on lines 56 to 59
- uses: coursier/setup-action@v1
with:
java-version: ${{ inputs.java-version }}
distribution: temurin
apps: scalafmt
jvm: temurin:${{ inputs.java-version }}
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what I did in the last push

.scalafmt.conf Outdated
# can't get fileOverride to work to specify scala3 as dialect for these
"scalalib/test/resources/hello-dotty/*"
# these have scaladoc comments with a particular shape that we don't want to reformat
"example/depth/large/*"
Copy link
Member

Choose a reason for hiding this comment

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

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

@alexarchambault alexarchambault force-pushed the actually-format-all-files branch 2 times, most recently from 1768f63 to 46c6379 Compare September 11, 2024 11:09
@alexarchambault alexarchambault marked this pull request as ready for review September 11, 2024 13:06
Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

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 alexarchambault changed the title Format all sources Format more sources Sep 12, 2024
@alexarchambault alexarchambault changed the title Format more sources Format using scalafmt command rather than ScalafmtModule/checkFormatAll task Sep 12, 2024
@alexarchambault alexarchambault marked this pull request as draft September 12, 2024 09:01
@alexarchambault
Copy link
Contributor Author

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

@lefou
Copy link
Member

lefou commented Sep 12, 2024

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
Copy link
Contributor Author

alexarchambault commented Sep 12, 2024

@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
Copy link
Member

lefou commented Sep 12, 2024

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

lefou pushed a commit that referenced this pull request Sep 13, 2024
Settings, see #3511

Pull request: #3526
@lihaoyi
Copy link
Member

lihaoyi commented Sep 17, 2024

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

This formats more sources, including those not known to the build
@alexarchambault
Copy link
Contributor Author

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

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.

3 participants