Autoformat Java examples

[myyk]

• Need to merge Apply Scalafmt to examples/ in CI #3903 first because it's built directly on top of it to reduce conflicts.

Change 2/3 for #3829

• I updated the PalantirFormatModule.scala so that it didn't create a ton of noise when there's actually no issues such as no java code changing. I think this is a good thing to do, but I'm not 100% since some users might feel this change in some way.
• Bugfix: The Github Action now fails on any error instead of only ./mill -i __.fix --check (was using ; instead of &&)
• Added Palantir formatting into the Github Action
• Removed a lot of cruft so that the example java modules could use the PalantirFormatModule.

[myyk]

@lihaoyi I think this one is ready for a review when you get a chance.

Update: I'm not sure how to update the linecounts in the failing tests. I'm having trouble understanding how those are run.

[lihaoyi]

Looks great, I think just need to update the test cases and we're good to go

[lihaoyi]

Looks like a bunch of PalantirFormatModuleTest tests are failing. I'm guessing those expect their test data to be mis-formatted, so we might need to figure out a way to skip those

[lihaoyi]

So PalantirJavaFormat does not provide any builtin flags to ignore files or folders, instead Mill manages the listing of files to pass to the tool.

mill/scalalib/src/mill/javalib/palantirformat/PalantirFormatModule.scala

Lines 173 to 181 in 4c05156

	args ++=
	sources
	.iterator
	.map(_.path)
	.flatMap(os.walk(_, includeTarget = true))
	.filter(os.isFile)
	.filter(_.ext == "java")
	.map(_.toString())

So we likely need to add some logic in Mill to allow PalantirJavaFormatModule to ignore particular globs, and then use that in Mill's own build (after rebootstrapping) to ignore the files and roll out enforcement internally.

To do this, I suggest we overload PalantirFormatModule.formatAll to take an additional optional parameter excludedGlobs: Seq[String]. We can then plumb those through to the java-file-listing above and use it to ignore files matching a glob (may need to find a glob-matching implementation online). We'd need to preserve the current formatAll signature for binary compatibility, but it can be a dumb forwarder to the new implementation

[myyk]

I had actually already implemented skipping some files from the .mill side but I guess I missed something.

…

On Sat, Nov 9, 2024, 15:40 Li Haoyi ***@***.***> wrote: So PalantirJavaFormat does not provide any builtin flags to ignore files or folders, instead Mill manages the listing of files to pass to the tool. https://github.com/com-lihaoyi/mill/blob/4c05156ccd8eca733411139b40886af7fcc485dc/scalalib/src/mill/javalib/palantirformat/PalantirFormatModule.scala#L173-L181 So we likely need to add some logic in Mill to allow PalantirJavaFormatModule to ignore particular globs, and then use that in Mill's own build (after rebootstrapping) to ignore the files and roll out enforcement internally. To do this, I suggest we overload PalantirFormatModule.formatAll to take an additional optional parameter excludedGlobs: Seq[String]. We can then plumb those through to the java-file-listing above and use it to ignore files matching a glob (may need to find a glob-matching implementation online). We'd need to preserve the current formatAll signature for binary compatibility, but it can be a dumb forwarder to the new implementation — Reply to this email directly, view it on GitHub <#3919 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAP4ANE3JV22YFYEZLKXVLLZ7W355AVCNFSM6AAAAABRLLUVESVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINRWGEYDIMBQGI> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[myyk]

The glob solution seems like a good way to go but I'm not sure when I can do it. Kotlin needs a similar mechanism and I think it would be good to have all the linter modules have roughly the same interface (where possible) which they don't currently.

…

On Sat, Nov 9, 2024, 15:43 Myyk Seok ***@***.***> wrote: I had actually already implemented skipping some files from the .mill side but I guess I missed something. On Sat, Nov 9, 2024, 15:40 Li Haoyi ***@***.***> wrote: > So PalantirJavaFormat does not provide any builtin flags to ignore files > or folders, instead Mill manages the listing of files to pass to the tool. > > > https://github.com/com-lihaoyi/mill/blob/4c05156ccd8eca733411139b40886af7fcc485dc/scalalib/src/mill/javalib/palantirformat/PalantirFormatModule.scala#L173-L181 > > So we likely need to add some logic in Mill to allow > PalantirJavaFormatModule to ignore particular globs, and then use that > in Mill's own build (after rebootstrapping) to ignore the files and roll > out enforcement internally. > > To do this, I suggest we overload PalantirFormatModule.formatAll to take > an additional optional parameter excludedGlobs: Seq[String]. We can then > plumb those through to the java-file-listing above and use it to ignore > files matching a glob (may need to find a glob-matching implementation > online). We'd need to preserve the current formatAll signature for > binary compatibility, but it can be a dumb forwarder to the new > implementation > > — > Reply to this email directly, view it on GitHub > <#3919 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAP4ANE3JV22YFYEZLKXVLLZ7W355AVCNFSM6AAAAABRLLUVESVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINRWGEYDIMBQGI> > . > You are receiving this because you authored the thread.Message ID: > ***@***.***> >

[lihaoyi]

Ah I just noticed your skipping code in Mill's own build. Not sure why it's not working

[myyk]

Oh interesting. I see you changed the sources in example/build.mill, is that what added a lot more linting to this? That found a lot of files I missed.

[lihaoyi]

@myyk yeah, previously we only used lintableJavaSources in PalantirFormatModule/, and didnt use it in ScalafmtModule/ so the Scala .mill files were not picked up. Furthermore, sources was pointed at testRepoRoot, which for the javalib/ examples contains a copy of the files and not the original ones so formatting them doesn't do anything.

By re-using the name sources, both PalantirFormatModule/ and ScalafmtModule/ pick up all of them, which is why now the .mill files in the Java folders are being picked up. And I re-targeted sources directly at millSourcePath, so it doesn't get affected by the override of testRepoRoot in ExampleCrossModuleJava

[myyk]

That all makes sense. Oh, I think I fixed some of that in the next change #3923 and didn't back port it because I thought it was working enough in this PR. Sorry about that. But also, I wasn't sure about fully redefining sources like that because I'm new to the project. That makes a lot of sense though and I'm glad for the help.