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

206 scalafmt is messing up diffs too much #207

Merged
merged 2 commits into from
Jul 6, 2023

Conversation

nawforce
Copy link
Contributor

@nawforce nawforce commented Jul 6, 2023

This adds sbt-scalafmt so we can use sbt scalafmtAll to update formatting quickly. I have also added sbt scalafmtCheck so the CI build should fail if we have not got scalafmt setup correctly when making changes.

The build changes are in the first commit, the second has reformatted sources.

@nawforce nawforce linked an issue Jul 6, 2023 that may be closed by this pull request
@nawforce nawforce requested a review from pwrightcertinia July 6, 2023 08:04
@pwrightcertinia
Copy link
Contributor

I am not sure why the plugin is necessary if it will be run separately to the build - you can run scalafmt on its own with the same features I think. It's now possible to integrate it into the build tasks at the beginning, but it might slow things down a bit.

I am planning to replace the existing git hook contents on the other repos with lint-staged so it can run all the different types of formatter if needed. I realise this repo does not have much of a js presence, so I can see why we might use a different way here.

@nawforce
Copy link
Contributor Author

nawforce commented Jul 6, 2023

I don't have scalafmt installed as a command line tool, I just use InteiliJ to take care of this. I could reformat all files that way but having a block in the build appeared important which looked to me like easier done via sbt plugin. I can drop those changes if you prefer and just correct all the errors, I have a PR open where I can't see the changes clearly because of the number of formatting changes. Github ignore whitespace helps with some but there are still quite a lot left.

@pwrightcertinia
Copy link
Contributor

pwrightcertinia commented Jul 6, 2023

Actually I've changed my mind - scalafmt is not available (on PATH atleast) by default in CI like sbt is. So this is decent, it wouldn't interfere with anything else either so no harm done. Sorry

Could be nice to run it as part of build, flip between check/all depending on CI=true environment

@nawforce nawforce merged commit b56121f into main Jul 6, 2023
@nawforce nawforce deleted the 206-scalafmt-is-messing-up-diffs-too-much branch July 6, 2023 18:50
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.

scalafmt is messing up diffs too much
2 participants