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

Adopt scalafmt for Scala code style #3841

Merged
merged 15 commits into from
Dec 8, 2023
Merged

Adopt scalafmt for Scala code style #3841

merged 15 commits into from
Dec 8, 2023

Conversation

Scott-Guest
Copy link
Contributor

@Scott-Guest Scott-Guest commented Dec 4, 2023

Closes #3827.

This PR adopts scalafmt for Scala code formatting.

Specifically,

  • Both google-java-format and scalafmt are now handled by the spotless Maven plugin
    • Run mvn spotless:check to check formatting (or mvn verify)
    • Run mvn spotless:apply to auto-format all source files (or mvn process-sources or mvn package)
  • CI runs mvn spotless:check to enforce formatting

Intellij has built-in support for scalafmt. Enable it by opening Settings > Editor > Code Style > Scala and setting

  • Formatter: Scalafmt
  • Configuration: k/src/main/config/.scalafmt.conf

The documentation on the different configuration options can be found here: https://scalameta.org/scalafmt/docs/configuration.html.

Currently, I've formatted with my preferred settings, but committed two versions:

  • 506616c, which does not enforce vertical alignment
  • 602fd2e, which does enforce vertical alignment

I'd appreciate if everyone who works on the frontend could skim through the files and let me know your thoughts!

@Scott-Guest Scott-Guest self-assigned this Dec 5, 2023
@Scott-Guest Scott-Guest marked this pull request as ready for review December 5, 2023 03:38
@Scott-Guest Scott-Guest requested a review from a team as a code owner December 5, 2023 03:38
Copy link
Contributor

@Baltoli Baltoli left a comment

Choose a reason for hiding this comment

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

I've skimmed through and this seems good to me; the pros of having a consistently-enforced Scala style outweigh any possible cons from a particular piece of code looking suboptimal.

I'd register a weak preference in favour of column alignment.

Copy link
Contributor

@radumereuta radumereuta left a comment

Choose a reason for hiding this comment

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

A few nitpicks regarding comments.
I'm in favor of code alignment.
lgtm otherwise.

case Rewrites(_, p1, p2) => symbols(p1) ++ symbols(p2)
case _ => Seq()
case _ => Seq()
Copy link
Contributor

Choose a reason for hiding this comment

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

I take it back. This looks nice.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Indeed, it is much more readable!

kore/src/main/scala/org/kframework/POSet.scala Outdated Show resolved Hide resolved
Copy link
Collaborator

@Robertorosmaninho Robertorosmaninho left a comment

Choose a reason for hiding this comment

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

This looks really nice! Thanks for your work @Scott-Guest!

I would advocate for the column align as well!

I made some comments about comments😅!
I'm not sure if we want to enforce the column alignment on them as well, but if we want to, we need to make sure they look as readable as they were before and that their meaning is the same!

I'll request a change at least for the kore/src/main/scala/org/kframework/unparser/Unparse.scala if you don't mind!

@Baltoli
Copy link
Contributor

Baltoli commented Dec 5, 2023

@dwightguth queries editor integrations - other than IntelliJ. Will be inconvenient if no Vim there's a CLI version

Copy link
Collaborator

@Robertorosmaninho Robertorosmaninho left a comment

Choose a reason for hiding this comment

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

LGTM!

@Robertorosmaninho
Copy link
Collaborator

@Scott-Guest, do you know why the Check Java Code Formatting isn't being executed for this PR?

@Baltoli
Copy link
Contributor

Baltoli commented Dec 6, 2023

@Scott-Guest, do you know why the Check Java Code Formatting isn't being executed for this PR?

An admin (@ehildenb, @dwightguth, @F-WRunTime) will need to amend the branch protection rules

Copy link
Contributor

@gtrepta gtrepta left a comment

Choose a reason for hiding this comment

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

This looks good to me regarding the scala code itself. I'm bearish on formatting comments, I think it's outside the scope of what can be expected from a formatter and there can be specific ways a coder would want to format their comments that a formatter might contend against. Those are my two cents, it sounds like everyone else is fine with it.

@Baltoli
Copy link
Contributor

Baltoli commented Dec 7, 2023

and there can be specific ways a coder would want to format their comments that a formatter might contend against

Yeah, I agree with this basically @gtrepta and I think it's a reasonable concern - for the formatters we've chosen:

  • I believe google-java-format is much stricter about formatting JavaDoc comments (/**), but will leave non-JavaDoc (/*) comments much more untouched. There are also escape hatches like <pre> for things like diagrams in JavaDocs.
  • There are loads more knobs to turn in Scalafmt; if it's really consistently mangling comments it's not a huge imposition to try and find a slightly different configuration to improve the situation.

@Scott-Guest
Copy link
Contributor Author

@gtrepta @Baltoli If need be, we can also configure spotless to disable formatting of certain sections using // spotless:on and // spotless:off

This is done at a higher level than the formatters (removing the section of code out of the file, running the formatter, then re-inserting it), so it works even with google-java-format which doesn't natively have this ability.

@rv-jenkins rv-jenkins merged commit 4269d27 into develop Dec 8, 2023
8 checks passed
@rv-jenkins rv-jenkins deleted the scalafmt branch December 8, 2023 04:26
rv-jenkins pushed a commit to runtimeverification/llvm-backend that referenced this pull request Dec 12, 2023
In runtimeverification/k#3841, we adopted the
`scalafmt` tool to auto-format the Scala code in the K frontend. This PR
adopts a precisely identical formatting setup for the LLVM backend's
Scala code.
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.

8 participants