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

Add 'scalafmt' to the project #1989

Merged
merged 10 commits into from
Jul 9, 2024
Merged

Conversation

satorg
Copy link
Contributor

@satorg satorg commented Feb 4, 2024

This PR:

  • Adds sbt-scalafmt plugin.
  • Creates .scalafmt.conf1.
  • Applies scalafmtSbt and +scalafmtAll commands2.
  • Creates .git-blame-ignore-revs to exclude commits with auto-reformatting and code auto-generation from git blame3.

  1. The .scalafmt.conf mostly contains default settings with a few exceptions. The exceptions are to try minimizing further reformatting. In a case if some specific settings are more preferable for the project, let me know please – I am open to accommodate any configuration changes in here.
  2. There's no manual formatting in the PR except the FreeGen2.scala file – the latter contains a lot of strings that turn into code and there are quite a lot of pretty long strings among them. Turns out scalafmt is not really great in such cases and can end up with weird formatting. Therefore, I added some manual reformatting so that after scalafmtSbt the reformatted code still looks pretty decent and readable. All the manually reformatted code is gathered in a single commit: fix formatting in FreeGen2.scala.
  3. If this branch gets rebased then the content of the .git-blame-ignore-revs file has to be updated accordingly.

@jatcwang
Copy link
Collaborator

jatcwang commented Mar 20, 2024

Thank you! I'll find an opportune time to run this and merge this in (to avoid conflicts in all other PRs)

@satorg
Copy link
Contributor Author

satorg commented Jun 10, 2024

@jatcwang , I've actualized the PR: rebased onto main, updated scalafmt version, re-applied formattings and .git-blame-ignore-revs

Let me know if you have any suggestions/concerns please.
Thank you!

@jatcwang
Copy link
Collaborator

Thanks @satorg. Originally I wanted to merge this after RC6 because I didn't know about .git-blame-ignore-revs. But now I think we can get this in before RC6. I have a change with quite some lines of changes I'm working on so I'll sort this one out after that 🙏

@satorg
Copy link
Contributor Author

satorg commented Jun 13, 2024

@jatcwang sure, np. Let me know please when I should update this PR to synchronize it with the upstream.

Also I think that once this PR gets settled in main, it would make sense to enforce scalafmt checks in CI/CD with a follow-up PR. I think I could take on that too – just let me know when to step in please. Thank you!

@jatcwang
Copy link
Collaborator

jatcwang commented Jun 14, 2024

@satorg I agree about the enforcement in CI. Feel free to implement it as part of this PR :)

@satorg
Copy link
Contributor Author

satorg commented Jun 20, 2024

The PR is updated:

  • Bump scalafmt up to v3.8.2
  • Reformat with the new version
  • add scalafmt checks to the CI workflow

@satorg
Copy link
Contributor Author

satorg commented Jun 20, 2024

@jatcwang , turns out the new Scalafmt version (3.8.2) started reformatting in multiline strings a little more aggressively.
It resulted in these changes: d8c3d1c

The changes don't break the build but look a bit awkward to me.
If you'd like to, I can either

  • leave it as is and therefore add the commit to .git-blame-ignore-revs (not there yet), OR
  • try to manually improve it (and keep outside of .git-blame-ignore-revs).

Wdyt?

@jatcwang
Copy link
Collaborator

Thanks @satorg. I agree it looks worse but happy to leave it as is, as that code will probably change as part of my work to fix #533

@satorg
Copy link
Contributor Author

satorg commented Jun 20, 2024

Looks like the CI failed because freeGen2 command updates auto-generated sources...

The problem here is that if I apply scalafmt to auto-generated sources and commit the changes (as it is done now in the PR), then freeGen2 reverts all the formatting and these files appear as uncommitted.

However, if I don't apply formatting to those files, then scalafmt checks will begin failing.

Perhaps the simplest way to resolve this conundrum could be excluding auto-generated files from scalafmt's scope. I'll try to accomplish it shortly.

@satorg
Copy link
Contributor Author

satorg commented Jun 21, 2024

The PR is updated:

  1. Disable formatting in auto-generated files via adding // format: off to the beginning of each file.
  2. Reformat again to apply the changes.

@satorg
Copy link
Contributor Author

satorg commented Jun 21, 2024

@jatcwang, hopefully, the CI issue has been resolved now.

@satorg
Copy link
Contributor Author

satorg commented Jun 21, 2024

@jatcwang , just in case – in regards to the auto-generated sources:

Perhaps, in makes sense to bring in an approach that is used in Cats for that:
https://github.com/typelevel/cats/blob/dd891334505b35e010a12da254924811de838c85/build.sbt#L114

I.e. all the auto-generated sources are written into directories like .../target/scala-2.13/src_managed/main and therefore never auto-formatted nor even committed to Git.

I feel, this PR is ready to go, so I would refrain from adopting this technique right in here. Just because doing that would require even more seemingly unrelated changes to this PR.

However, if you like such an approach and don't mind landing it in this repo, I could take on it in a follow-up PR.

# Conflicts:
#	build.sbt
#	project/plugins.sbt
@jatcwang
Copy link
Collaborator

jatcwang commented Jul 8, 2024

@satorg Yes I do agree that we should use sourceGenerators. I did try it before when I was improving/fixing the code generation but I can't remember what was the show stopper 🤔 Perhaps I wanted to keep the generated code committed in git so people can look at it on github.

@jatcwang jatcwang merged commit d8a2afd into typelevel:main Jul 9, 2024
6 checks passed
@satorg satorg deleted the introduce-scalafmt branch July 9, 2024 07:40
@jatcwang
Copy link
Collaborator

jatcwang commented Jul 9, 2024

Big thanks for sorting this out @satorg!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants