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

Apply Scalafmt to examples/ in CI #3903

Merged
merged 10 commits into from
Nov 8, 2024
Merged

Conversation

myyk
Copy link
Contributor

@myyk myyk commented Nov 4, 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 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.

.scalafmt.conf Show resolved Hide resolved
@lihaoyi
Copy link
Member

lihaoyi commented Nov 5, 2024

Looks great as a first pass, some config tweaks would allow us to include the .mill files and disable the comment formatting for those in example/, which would allow us to skip the // format: off comment in each one necessary to preserve the magic usage comments

@myyk

This comment was marked as outdated.

@lihaoyi
Copy link
Member

lihaoyi commented Nov 7, 2024

@myyk ah that's the parser not liking the Scala 3 syntax used in that file. I think you should be able to just skip those files, or use fileOverride config to tell ScalaFmt to use Scala 3 syntax

@@ -254,6 +254,7 @@ object TestModuleUtilTests extends TestSuite {
)
)
}
// format: off
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did this because it keeps fucking up the format inside this function. format didn't seem to turn off on a llne based usage.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good, maybe something to do with the XML literals which are pretty obscure and mayb the formatter doesn't work well with

@myyk
Copy link
Contributor Author

myyk commented Nov 7, 2024

@lihaoyi I applied all the feedback given. I'm much happier with this version.

Copy link
Member

@lihaoyi lihaoyi left a comment

Choose a reason for hiding this comment

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

Looks good to me, CC @lefou in case you want to review this before we merge it

@myyk myyk mentioned this pull request Nov 7, 2024
1 task
@lihaoyi lihaoyi merged commit 4f70fd6 into com-lihaoyi:main Nov 8, 2024
25 checks passed
@myyk myyk deleted the lint-examples branch November 8, 2024 07:35
@lefou lefou added this to the 0.12.2 milestone Nov 9, 2024
lihaoyi added a commit that referenced this pull request 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 <haoyi.sg@gmail.com>
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