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

Spotless instead of intellij formatting #306

Merged
merged 10 commits into from
Oct 21, 2024
Merged

Spotless instead of intellij formatting #306

merged 10 commits into from
Oct 21, 2024

Conversation

leventeBajczi
Copy link
Contributor

@leventeBajczi leventeBajczi commented Oct 19, 2024

This PR replaces the automatic format- and copyright checks, which currently mostly relied on IntelliJ IDEA's format.sh script, with the spotless project.

The main reason for this change is that a simple version update (from 2023.x to 2024.x) modified how the code style is applied to Java and Kotlin files. I think this is unacceptable.

Further advantages of spotless:

  • It is easy to reformat code offline (using ./gradlew spotlessApply)
  • It would be easy to set up a pre-commit hook (example) to check for unformatted code, not requiring a push and CI check
  • It uses well-established formatters such as google-java-format
  • It can use ratcheting to allow incremental formatting, not requiring a reformat in the entire repo

Some disadvantages:

  • Spotless currently does not support intellij code style XMLs
  • We would either need to create an Eclipse code style file to use custom rules, or we would be stuck with defaults (such as google's java style) -- although this is considered an advantage by many, as using commonly used formatting guidelines can be easier on new contributors

This PR also reverts recent changes in e37b323 and 185dc7a. These are recent formatting commits that should never happen again. This will hopefully fix merge issues before they are dealt with in e.g. #304.

The formatting rules can be found in java-common.build.kts. I'm open to any suggestion on what formatters, with which options to enable. Please check the plugin's README for options.

I would expect some insights from recent contributors to Theta, such as @mondokm, @RipplB, @AdamZsofi, @szdan97, @as3810t, @csanadtelbisz, @s0mark (Sorry if I missed anyone!)

(Note that on this PR, the checks will most likely fail. I deliberately don't want to re-edit the reverted files.)

@AdamZsofi
Copy link
Member

So, if I understand correctly, the current state of formatting stays for all files, except the ones that will be changed by later commits.
Sounds good to me, definitely better than what happened with the reverted commit and the intellij update.

It's hard to imagine the new formatting from just config files, would it be possible to see the difference on a few example files? @leventeBajczi

@leventeBajczi
Copy link
Contributor Author

@AdamZsofi: good idea, I pushed the spotless-demo-DONOTMERGE branch for demo purposes: changes

@leventeBajczi leventeBajczi merged commit 1d6d1ea into master Oct 21, 2024
13 of 33 checks passed
@leventeBajczi leventeBajczi deleted the spotless branch October 21, 2024 17:57
Copy link

Benchexec test report for a selection of SV-Benchmarks (correct / incorrect / all):

task set BOUNDED CEGAR HORN
Arrays ✅ (1 / 0 / 83) HTML/CSV ❓ (0 / 0 / 4) HTML/CSV ✅ (1 / 0 / 52) HTML/CSV
BitVectors ✅ (4 / 0 / 21) HTML/CSV ✅ (6 / 0 / 30) HTML/CSV ✅ (2 / 0 / 30) HTML/CSV
Combinations ❓ (0 / 0 / 39) HTML/CSV ❓ (0 / 0 / 39) HTML/CSV ❓ (0 / 0 / 39) HTML/CSV
Concurrency ❓ (0 / 0 / 113) HTML/CSV ✅ (7 / 0 / 40) HTML/CSV ❓ (0 / 0 / 111) HTML/CSV
ConcurrencySafety-NoOverflows ❓ (0 / 0 / 108) HTML/CSV ✅ (6 / 0 / 35) HTML/CSV ❓ (0 / 0 / 108) HTML/CSV
ControlFlow ✅ (3 / 0 / 31) HTML/CSV ✅ (2 / 0 / 54) HTML/CSV ❗ (31 / 17 / 66) HTML/CSV
ECA ❓ (0 / 0 / 36) HTML/CSV ❓ (0 / 0 / 36) HTML/CSV ❓ (0 / 0 / 37) HTML/CSV
Floats ❓ (0 / 0 / 17) HTML/CSV ✅ (7 / 0 / 22) HTML/CSV ❓ (0 / 0 / 33) HTML/CSV
Hardware ❓ (0 / 0 / 28) HTML/CSV ✅ (1 / 0 / 11) HTML/CSV ❓ (0 / 0 / 28) HTML/CSV
Heap ✅ (10 / 0 / 112) HTML/CSV ✅ (41 / 0 / 90) HTML/CSV ❗ (10 / 2 / 112) HTML/CSV
Loops ✅ (12 / 0 / 50) HTML/CSV ✅ (1 / 0 / 7) HTML/CSV ✅ (24 / 0 / 69) HTML/CSV
NoDataRace ❓ (0 / 0 / 117) HTML/CSV ✅ (27 / 0 / 49) HTML/CSV ❓ (0 / 0 / 117) HTML/CSV
ProductLines ❓ (0 / 0 / 315) HTML/CSV ❓ (0 / 0 / 318) HTML/CSV ❓ (0 / 0 / 315) HTML/CSV
Recursive ❓ (0 / 0 / 153) HTML/CSV ✅ (7 / 0 / 36) HTML/CSV ❓ (0 / 0 / 153) HTML/CSV
Sequentialized ✅ (2 / 0 / 31) HTML/CSV ✅ (2 / 0 / 31) HTML/CSV ✅ (3 / 0 / 30) HTML/CSV
XCSP ❓ (0 / 0 / 19) HTML/CSV ❓ (0 / 0 / 19) HTML/CSV ❓ (0 / 0 / 19) HTML/CSV

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.

2 participants