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

Replace scalafmtOnCompile with CI check #457

Merged
merged 1 commit into from
Nov 3, 2024

Conversation

mdedetrich
Copy link
Contributor

@mdedetrich mdedetrich commented Oct 18, 2024

This PR replaces scalafmtOnCompile with a github action that runs scalafmt-native that checks if the entire codebase is formatted. The advantage of this versus using scalafmtOnCompile is it doesn't mess up editors while developing and since it is its own github action check it runs in parallel to the main sbt build (this also means it will even run if the sbt fails to load/compile). The scalafmt-native check also only runs on files that are different to origin/develop and since its running in native the entire github check typically only takes around 5-10 seconds.

Most critically it ensures that the code is actually properly formatted since with the current setup, its still possible to push unformatted code just by simply forgetting to compile before pushing (which is what happened before).

The only thing that needs to be done after this PR is merged is to add Scalafmt / Code is formatted as a github CI status check so that you cannot merge PR's if they are not formatted.

@mdedetrich mdedetrich force-pushed the replace-scalafmtoncompile-with-ci branch 2 times, most recently from d7f1d92 to 424ced1 Compare October 18, 2024 10:22
@Friendseeker
Copy link
Member

Friendseeker commented Nov 1, 2024

This looks fantastic. I am not too sure about disabling scalafmtOnCompile (I use Intellij and scalafmtOnCompile never caused any issue) but adding a CI job to check formatting is certainly very welcomed.

@Friendseeker
Copy link
Member

Friendseeker commented Nov 1, 2024

Just want to confirm, does the new CI job also catch an ill-formatted build.sbt file? If so then the PR LGTM.

@Friendseeker
Copy link
Member

Friendseeker commented Nov 1, 2024

Wait looks like there's already a scalafmt check as part of CI. If it does the same thing as the PR, I guess the PR is no longer necessary then.

- name: Build and test (1)
if: ${{ matrix.jobtype == 1 }}
shell: bash
run: |
sbt -v -Dfile.encoding=UTF8 scalafmtCheckAll +test +packagedArtifacts


Ehh Github does not have a button to rollback PR approval...

Copy link
Member

@Friendseeker Friendseeker left a comment

Choose a reason for hiding this comment

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

Due to #457 (comment), reserving approval for now.

@mdedetrich
Copy link
Contributor Author

Wait looks like there's already a scalafmt check as part of CI. If it does the same thing as the PR, I guess the PR is no longer necessary then.

Let me change it, sec

@mdedetrich mdedetrich force-pushed the replace-scalafmtoncompile-with-ci branch 2 times, most recently from 5e39336 to 7fb67a2 Compare November 2, 2024 08:50
@mdedetrich
Copy link
Contributor Author

@Friendseeker Done

@Friendseeker
Copy link
Member

Friendseeker commented Nov 2, 2024

@Friendseeker Done

Thanks! I am not very familiar with the repo as evidenced by me initially overlooking the existence of 'scalafmtcheckAll'. Probably for the best to let Eugene or other people more experienced with the repo to proceed with the review.

build.sbt Show resolved Hide resolved
@mdedetrich mdedetrich force-pushed the replace-scalafmtoncompile-with-ci branch from 7fb67a2 to 30584d5 Compare November 3, 2024 09:10
Copy link
Member

@eed3si9n eed3si9n left a comment

Choose a reason for hiding this comment

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

Thanks!

@eed3si9n eed3si9n merged commit 949dd17 into sbt:develop Nov 3, 2024
5 checks passed
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