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

Added plugin for formatting Java sources using Palantir #3531

Merged
merged 27 commits into from
Sep 22, 2024

Conversation

ajaychandran
Copy link
Contributor

@ajaychandran ajaychandran commented Sep 12, 2024

Added contrib.palantir.JavafmtModule for formatting Java sources using Palantir.
The plugin also supports command line arguments for

  • checking for formatting errors with --check flag
  • formatting specific files or folders

Resolves #3448.

@ajaychandran
Copy link
Contributor Author

@lihaoyi
Is the naming convention fine?
Are there any additional features that must be implemented?

contrib/package.mill Outdated Show resolved Hide resolved
@lihaoyi
Copy link
Member

lihaoyi commented Sep 12, 2024

Looks good overall, just need a few tweaks. Need a short readme.adoc in the contrib module folder, and an example test in example/javalib/linting/ next to checkstyle and errorprone to demonstrate usage.

@ajaychandran
Copy link
Contributor Author

@lihaoyi Please review.

  • The JVM args for Java 16+ do not seem to affect the behavior when using a lower Java version (tested with Java 11).

@ajaychandran ajaychandran marked this pull request as ready for review September 18, 2024 13:37

> ./mill javafmt # alternatively, format all Java source files

*/
Copy link
Member

@lihaoyi lihaoyi Sep 19, 2024

Choose a reason for hiding this comment

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

We should also show off the ./mill mill.contrib.palantirjavaformat.PalantirJavaFormat/ workflow, and after formatting is run we should run check again and ensure it does not error like it did before

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. Please review.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to remove check examples.

 Incomplete argument --check <bool> is missing a corresponding value
  Expected Signature: palantirformat
    --check <bool>
    sources <str>...
  

  ,)

@@ -0,0 +1,187 @@
package mill
package contrib.palantirjavaformat
Copy link
Member

Choose a reason for hiding this comment

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

I edited a comment earlier to say this, but let's standardize the naming convention of palantirformat/PalantirFormat. I know it doesn't quite match the palantir-java-format project name, but using the full name over and over is getting verbose enough I think we should shorten it here just for ease of use

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lihaoyi
How about palantirjavafmt? Get a lot more clarity with the addition of a single character.

Copy link
Member

Choose a reason for hiding this comment

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

Let's go with palantirformat, googling palantir format brings up only this one project, so there shouldn't be any ambiguity for now

Copy link
Member

Choose a reason for hiding this comment

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

One consideration is that I may want to move this directly into javalib, just like how ScalaFmtModule is in scalalib and KLintModule is in kotlinlib. That way it'll be mill.javalib.palantirformat which should be unambiguous enough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where would the documentation go in this case? docs/modules/ROOT/pages/Java_Module_Config.adoc?

@lihaoyi
Copy link
Member

lihaoyi commented Sep 19, 2024

Left some comments

The tests are failing because we need to wire up contrib.palantirformat into our dist so it will get properly published during integration testing. Should just involve adding it to the list here along with the other integration-tested contrib modules

mill/build.mill

Line 727 in 2dc343f

def testTransitiveDeps = build.runner.testTransitiveDeps() ++ Seq(

@lihaoyi
Copy link
Member

lihaoyi commented Sep 21, 2024

I think this looks great, two small nits in the example and I think we can merge this and close out the bounty

@lihaoyi lihaoyi merged commit 09d7784 into com-lihaoyi:main Sep 22, 2024
23 of 24 checks passed
@lihaoyi
Copy link
Member

lihaoyi commented Sep 22, 2024

Thanks @ajaychandran! will transfer the bounty using the same details as earlier

@ajaychandran ajaychandran deleted the palantir branch September 22, 2024 03:56
@lefou lefou added this to the 0.12.0 milestone Sep 23, 2024
@0xnm 0xnm mentioned this pull request Sep 27, 2024
lihaoyi pushed a commit that referenced this pull request Sep 28, 2024
Fixes #3612.

This PR is based on the logic done in #3531, so it looks quite similar.

Co-authored-by: 0xnm <[email protected]>
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.

Support Palantir-Java-Format for Java (500USD Bounty)
3 participants