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

Switch out spotless for an alternative #1530

Merged
merged 24 commits into from
Dec 13, 2024

Conversation

lukebemish
Copy link
Contributor

@lukebemish lukebemish commented Sep 9, 2024

Spotless has... some quirks. Among others:

  • the author dislikes gradle's configuration cache conceptually, and spotless's support for it is generally iffy, including using some... questionable workarounds
  • a good bit of formatting logic is dependent on the JVM used to invoke gradle, and cannot change with the toolchain -- this is... not optimal, when you'd like a neo-dev environment to be pretty easy to have Just Work on most machines with a somewhat recent JVM present locally.
  • Sometimes its cache gets... wacky, and just doesn't recognize files as needing re-formatted
  • Running gradle with a different java version than you are developing for causes... issues, due to how spotless invokes formatters.

immaculate is a spotless alternative built with a much narrower scope (namely, the sort of formatting you tend to see in java projects like this), designed to utilize built-in gradle features when possible (up-to-date checking and incremental tasks for figuring out which files to re-check), prefer lazy configuration, and respect gradle best practices whereever I have any clue what they are; this PR switches neo-dev to use it instead of spotless. Note that, due to using the same version of eclipse JDT and the same other formatting steps besides that, no formatting has shifted compared to using spotless.

immaculate is new and thus rather untested; spotless is a much more established plugin, but also has a much more established set of issues, some of which seem to be continually popping up in discord (the notable one here is the whole bit where tons of stuff is dependent on the java version used to invoke gradle). Furthermore, this change could probably use some more extensive testing to make sure it behaves right in a number of different development environments.

In any case, this PR could use testing on different environments to make sure it's all behaving sensibly across dev environments.

@neoforged-pr-publishing
Copy link

  • Publish PR to GitHub Packages

@sciwhiz12 sciwhiz12 added enhancement New (or improvement to existing) feature or request ci/build Related to continuous integration/build system 1.21.1 Targeted at Minecraft 1.21.1 labels Sep 9, 2024
@lukebemish lukebemish marked this pull request as ready for review September 17, 2024 02:13
Copy link
Member

@sciwhiz12 sciwhiz12 left a comment

Choose a reason for hiding this comment

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

Untested yet, but looks clean enough. I have a few spots that need some explanation, though.

@sciwhiz12 sciwhiz12 self-requested a review November 18, 2024 08:11
Fixed to be unnecessary by new NG versions
@neoforged-automation
Copy link

@lukebemish, this pull request has conflicts, please resolve them for this PR to move forward.

@Technici4n Technici4n removed the 1.21.1 Targeted at Minecraft 1.21.1 label Dec 12, 2024
Technici4n
Technici4n previously approved these changes Dec 12, 2024
docs/CONTRIBUTING.md Outdated Show resolved Hide resolved
@Technici4n Technici4n merged commit df48b4d into neoforged:1.21.x Dec 13, 2024
6 checks passed
@neoforged-releases
Copy link

🚀 This PR has been released as NeoForge version 21.4.22-beta.

@lukebemish lukebemish deleted the no-more-spotless branch December 13, 2024 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/build Related to continuous integration/build system enhancement New (or improvement to existing) feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants