-
Notifications
You must be signed in to change notification settings - Fork 7
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
Run scalafmt in CI and in pre-commit hook #1710
Open
emdash-ie
wants to merge
5
commits into
main
Choose a base branch
from
run-scalafmt-in-CI
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
+9,886
−4,542
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Now that this repo is using prettier to check formatting in CI, why not also check scala formatting? This commit adds a check in CI that runs scalafmt via an sbt plugin, copying the approach from guardian/datawrapper-import#12 I’ll also add a pre-commit hook shortly, and format the existing code. I’ll probably choose a formatting style that uses tabs for indentation, to be consistent with the frontend code (and given the accessibility motivations for using tabs for indentation).
Scalafmt needs this file in order to run! These are the minimal config values required. Unfortunately, it looks like scalafmt doesn’t support using tabs for indentation: we’ll go with spaces for now but I’ll raise an issue with them requesting a config for using tabs. I reckon there are strong accessibility arguments in favour of tabs, so I’m hopeful I can convince them.
emdash-ie
force-pushed
the
run-scalafmt-in-CI
branch
2 times, most recently
from
October 25, 2024 11:52
ccf2c55
to
61f9ffa
Compare
Here’re the results of testing the updated pre-commit hook: git status
git diff (unstaged changes)
git diff --staged (staged changes)
Hook output
|
This commit updates the pre-commit hook to run scalafmt the same way it already runs prettier. Apart from the exit code tweak, this basically involved copying the pre-commit hook from guardian/datawrapper-import#12 As with the prettier part of the hook, this should only run the formatter on files with staged changes that don’t have unstaged changes, and for any with both that fail formatting it should just report an error and exit. (I’ve also replaced the word “clobbering” with “overwriting”, because I think it’s a less obscure phrasing.)
emdash-ie
force-pushed
the
run-scalafmt-in-CI
branch
from
October 25, 2024 11:52
61f9ffa
to
cf7464c
Compare
emdash-ie
changed the title
Run scalafmt in CI
Run scalafmt in CI and in pre-commit hook
Oct 25, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What's changed?
Similar to the recent changes to run prettier in CI and in a pre-commit hook, this PR configures scalafmt to do the same.
More information in the individual commit messages, reproduced here:
bc94ddd Run scalafmt in CI
Now that this repo is using prettier to check formatting in CI, why not also check scala formatting? This commit adds a check in CI that runs scalafmt via an sbt plugin, copying the approach from https://github.com/guardian/datawrapper-import/pull/12
I’ll also add a pre-commit hook shortly, and format the existing code. I’ll probably choose a formatting style that uses tabs for indentation, to be consistent with the frontend code (and given the accessibility motivations for using tabs for indentation).
6cf11c6 Add .scalafmt.conf
Scalafmt needs this file in order to run! These are the minimal config values required.
Unfortunately, it looks like scalafmt doesn’t support using tabs for indentation: we’ll go with spaces for now but I’ll raise an issue with them requesting a config for using tabs. I reckon there are strong accessibility arguments in favour of tabs, so I’m hopeful I can convince them.
96296b5 Run scalafmt on everything
241199e Add formatting commit to .git-blame-ignore-revs
cf7464c Update pre-commit hook to check scala formatting
This commit updates the pre-commit hook to run scalafmt the same way it already runs prettier. Apart from the exit code tweak, this basically involved copying the pre-commit hook from https://github.com/guardian/datawrapper-import/pull/12
As with the prettier part of the hook, this should only run the formatter on files with staged changes that don’t have unstaged changes, and for any with both that fail formatting it should just report an error and exit.
(I’ve also replaced the word “clobbering” with “overwriting”, because I think it’s a less obscure phrasing.)
Implementation notes
I’ve used scalafmt directly for the pre-commit hook rather than via the sbt plugin because sbt startup is quite slow, and it can be quite annoying to run it on every commit. Running scalafmt directly is a good bit faster.
Checklist
General
Client