Update and apply scalafmt

[mdedetrich]

Description

This PR updates the scalafmt config to the newest version as well as replacing a pre commit git hook with a github workflow action. This may be subjective but I have a personal hate with putting formatters within githooks because it really messes with IDE's especially in large codebases. Instead the github workflow action

• Runs concurrently to the main build and uses the scala-native version of scalafmt. This means its ultra fast, taking only around 2-3 seconds on small projects
• You can add the check to the list of branch checks that must pass before merging a PR to enforce not being able to merge unformatted code, i.e.

Type of change

Updating of scalafmt

Checklist:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• Any dependent changes have been merged and published in downstream modules
• I have checked my code and corrected any misspellings

[paul-a-kennedy-rally]

LGTM

[jeffmay]

I'm not sure I agree with removing pre-commit-hooks. I would prefer just running pre-commit manually with pre-commit run -a whenever you want and just verifying that pre-commit succeeds with a github workflow.

[jeffmay]

This may be subjective but I have a personal hate with putting formatters within githooks because it really messes with IDE's especially in large codebases.

Which IDE do you use? I use IntelliJ and you can tell it to not run git hooks. Also, you don't have to install pre-commit hooks locally.

[jducoeur]

No longer at Rally myself, but from my time there, I found pre-commit-hooks absolutely invaluable. In combination with scalafmt and a properly-configured IntelliJ, it meant that I almost never needed to think about formatting, which was a non-trivial time-saver.

[mdedetrich]

Regarding me personally when I did have them available it slowed down my workflow especially in large projects (especially with Scala due to sbt startup time). I typically only ran a scalafmt just before I was going to push something where as locally I may commit many times quickly. In tandem with the github actions scalafmt which takes a few seconds to complete on CI, if I did happen to forget to format I pretty got immediate feedback just when creating the PR.

In any case I am not going to die on this hill so I will rebase the PR to undo the removal of the pre commit hook.

[mdedetrich]

@jducoeur @jeffmay I have reverted the removal of the pre commit git hook

[jeffmay]

Regarding me personally when I did have them available it slowed down my workflow especially in large projects (especially with Scala due to sbt startup time). I typically only ran a scalafmt just before I was going to push something where as locally I may commit many times quickly. In tandem with the github actions scalafmt which takes a few seconds to complete on CI, if I did happen to forget to format I pretty got immediate feedback just when creating the PR.

In any case I am not going to die on this hill so I will rebase the PR to undo the removal of the pre commit hook.

Maybe we can look into using sbt --client in our own pre-commit-hook. That way, if you have the native client running for other reasons, then it will use the same sbt instance.

Also, if you prefer, you don't have to install pre-commit locally and only run it manually when you want to finalize a commit with pre-commit run -a

Also, we should definitely be validating pre-commit in Github Actions, so thank you for adding that check!

[jeffmay]

Hmm...

scalafmt-native --list
Error: Command failed: /home/runner/bin/scalafmt-native --list

[mdedetrich]

@jeffmay You can check the failure in the actual github action at https://github.com/rallyhealth/scalacheck-ops/actions/runs/3221009924/jobs/5268494582 i.e.

scalafmt-native --list
  build.sbt
  
  ::endgroup::
Error: Command failed: /home/runner/bin/scalafmt-native --list

You failed to format build.sbt (likely because you changed it). scalafmtSbt along with git commit amend should solve the issue

[mdedetrich]

So scalafmt pipeline failed again, are you doing scalafmtSbt locally?

[jeffmay]

I wonder how we can bridge the gap between the github action and pre-commit-conifg... I want to be able to run hooks that will autoformat the code (quickly) so that it passes CI. I think the "quickly" part rules out the old pre-commit-hooks that requires running a new JVM and sbt (not to mention it didn't even run scalafmtSbt). I wonder if we can run scalafmt-native from pre-commit the same way as github actions.

[mdedetrich]

I wonder how we can bridge the gap between the github action and pre-commit-conifg... I want to be able to run hooks that will autoformat the code (quickly) so that it passes CI. I think the "quickly" part rules out the old pre-commit-hooks that requires running a new JVM and sbt (not to mention it didn't even run scalafmtSbt). I wonder if we can run scalafmt-native from pre-commit the same way as github actions.

I guess you can try sbt in thin client mode (see https://www.scala-sbt.org/1.x/docs/sbt-1.4-Release-Notes.html#Native+thin+client), that should reuse the JVM process. Even though it doesn't work with environment variables I don't think thats relevant here.

Or as you said you could also just include the binaries of scalafmt-native in the git repo however it only supports Linux/Mac (see https://scalameta.org/scalafmt/docs/installation.html#native-image) and it also presents its own host of problems.

[jeffmay]

@mdedetrich it was easier than I thought. pre-commit supports docker images directly, so I could just use this: https://hub.docker.com/r/scalameta/scalafmt/tags

[jeffmay]

Remove. This was for testing

[paul-a-kennedy-rally]

Still something wonky going on: https://github.com/rallyhealth/scalacheck-ops/actions/runs/3235997094/jobs/5301188323

[paul-a-kennedy-rally]

LGTM

[paul-a-kennedy-rally]

Still something wonky going on: https://github.com/rallyhealth/scalacheck-ops/actions/runs/3235997094/jobs/5301188323

Well, I re-ran it and it was fine... hopefully not a source of flakiness.

[jeffmay]

Still something wonky going on: https://github.com/rallyhealth/scalacheck-ops/actions/runs/3235997094/jobs/5301188323

Well, I re-ran it and it was fine... hopefully not a source of flakiness.

Weird... looks like it had trouble downloading sbt... hopefully a rare issue.