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

Enforce ScalaFmt/PalantirJavaFormat/KtLint on example doc/test suites (500USD Bounty) #3829

Open
lihaoyi opened this issue Oct 24, 2024 · 7 comments
Labels

Comments

@lihaoyi
Copy link
Member

lihaoyi commented Oct 24, 2024


From the maintainer Li Haoyi: I'm putting a 500USD bounty on this issue, payable by bank transfer on a merged PR implementing this.


Currently managing the formatting for this is manual, and sometimes inconsistencies slip through. We should really just automate enforcement and compliance.

We should as much as possible re-use the infrastructure Mill has already built around these autoformatters, to dogfood it. This would mean doing the autoformatting as part of Mill's own Mill build, using the various ScalaFmtModule/PalantirJavaFormatModule/KtLintModules as necessary

@lihaoyi lihaoyi added the bounty label Oct 24, 2024
@lihaoyi lihaoyi changed the title Enforce ScalaFmt/PalantirJavaFormat/KtLint on example doc/test suites Enforce ScalaFmt/PalantirJavaFormat/KtLint on example doc/test suites (500USD Bounty) Oct 24, 2024
@LevisNgigi
Copy link

Hello interesting but I think this might not be beginner friendly for someone new to the project.Will be on the lookout for other issues.

@Sailsman63
Copy link

In this context, what does "Automate enforcement and compliance" look like?

  • Just surface the errors in the build script CI, block PRs on fixing them
  • Automatically apply and commit fixes on merge
  • Other?

@lihaoyi
Copy link
Member Author

lihaoyi commented Oct 26, 2024

Just surface the errors in the build script CI, block PRs on fixing them

Just this, with some manual command that can be run to fix them locally. Same as the current workflow.

@myyk
Copy link
Contributor

myyk commented Nov 3, 2024

I'm going to see if I can figure this one out tomorrow

@lihaoyi
Copy link
Member Author

lihaoyi commented Nov 3, 2024

@myyk some pointers if you're going to look into this:

  1. This should be purely a change in the build.mill/package.mill files, shouldn't need any application code changes

  2. The default mill.scalalib.scalafmt.ScalafmtModule defaults to looking for __.sources, so the easiest thing to do would be to hook into that by defining a sources in ExampleCrossModule that points to testRepoRoot so it can get picked up by ScalafmtModule/formatAll and checkFormatAll

  3. To make this work for Java sources as well, we would need to run PalantirFormatModule in addition to ScalafmtModule in CI here

  4. The example tests rely on comments being formatted in specific ways to be parsed and used as test commands. We may need to tweak add docstrings.style = keep to our .scalafmt.conf to preserve them as is

@myyk
Copy link
Contributor

myyk commented Nov 3, 2024

Thanks! I was trying to piece together 2, that helps a lot. That all makes sense.

@myyk

This comment was marked as outdated.

myyk added a commit to myyk/mill that referenced this issue Nov 7, 2024
lihaoyi pushed a commit that referenced this issue Nov 8, 2024
Partially completes: #3829

Tested locally by breaking format of
`example/scalalib/basic/1-simple/foo/src/Foo.scala` by just adding a
bunch of spaces.

* Observe break with `./mill
mill.scalalib.scalafmt.ScalafmtModule/checkFormatAll __.sources`
failure.
* Observe fix with `./mill
mill.scalalib.scalafmt.ScalafmtModule/checkFormatAll __.sources`

Used `// format: off` rather than changing [docstrings.style =
keep](https://scalameta.org/scalafmt/docs/configuration.html#docstringsstyle--keep)
to our `.scalafmt.conf` since that would affect all files. There doesn't
seem to be a built in way in ScalaFmt to have folder level configs or
something like this. I thought it might be better to not have `example`
be an exception cases if we can avoid it. I'm not sure if the `//
format: off` comments break something else though.
This was referenced Nov 8, 2024
lihaoyi added a commit that referenced this issue Nov 10, 2024
* [x] Need to merge #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`.

---------

Co-authored-by: Li Haoyi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants