neoforged / MDK

The Mod Developer Kit - this is where you start if you want to develop a new mod
https://github.com/NeoForgeMDKs
185 stars 59 forks source link

Add spotless to the MDK #52

Open Shadows-of-Fire opened 5 months ago

Shadows-of-Fire commented 5 months ago

This PR adds spotless to the MDK and applies the formatting to the two files currently present in the MDK. The rules used are identical to the rules used by NF, but does not include the custom rules (preventing wildcard imports and @NotNull).

TelepathicGrunt commented 5 months ago

What is the reasoning for this? Fighting spotless in Neo PRs is annoying. I rather swallow a cactus than to start fighting spotless in my own mods. Can't imagine how much pain beginners would have if they encounter this in the MDK and have no idea what it is or how to turn it off.

TelepathicGrunt commented 5 months ago

Another point of view as well, the MDK’s goal is to help give modders a starting point into modding for Neoforge. Spotless is not a requirement for modding (most modders don’t even use spotless) and thus, it’s out of scope for the MDK

lukebemish commented 5 months ago

I am against this change; in addition to the reasons given above, the spotless plugin is really janky and definitely does not follow gradle good practice -- it also requires users to use the same JVM to run gradle as they use in their source code, which is currently not a requirement of neogradle and not one we should force onto users. Basically, spotless is fine for neo internal use, but is just too much complexity and too weird of a system to be in the MDK. Users should not have to worry about gradle implementation details. Spotless is the sort of thing that forces you to worry about gradle implementation details.

Furthermore, the included formatting file is highly opinionated. Neo should not be shoving its code formatting opinions onto the MDK (and thus onto any modders that use it); ignoring the differing opinions in code format, this is a good way to get confusions about why you can't run ./gradlew build when your code is all just fine.

dhyces commented 5 months ago

If anything, this should be opt-in, with the plugin and config block commented out with explanations of the pros and cons.

sciwhiz12 commented 5 months ago

I am against this change as well. Adding Spotless can make a beginner developer's experience with our MDK more frustrating, if they write code and learn only afterwards while trying to build their mod that their code is in violation with some formatting rules they didn't really know existed.

Shadows-of-Fire commented 5 months ago

What is the reasoning for this?

Consistency in formatting for the files included with the MDK (the current files have arbitrary formatting) as well as an implied default for use across the board. The first is a local benefit to us, but the second means that it is easier to transition between reading the code of different projects, which makes it easier to assist users who have questions that requires browsing source code to resolve. It is unlikely that users stumble upon or implement spotless themselves (as the setup cost is fairly high, and designing the formatting file is nontrivial).

Fighting spotless in Neo PRs is annoying.

I can agree with that. I think that's a failure on our part to provide setup instructions for a pre-commit hook that applies spotless to the project, because if we did that nobody would ever need to think about it.

Based on this discussion it seems like it may be best to ship it with enforceCheck false (described here), which means it will not fail the build step for spotless violations.
Applying this change will make it opt-in for enforcement, but still available as a formatting tool by running the spotlessApply task.

lukebemish commented 5 months ago

Irregardless of that, I am still against including spotless in the MDK due to the requirement it sometimes enforces on using the same java version to run gradle as to run the game, as well as the jankiness of it's gradle setup in general

lukebemish commented 5 months ago

Additionally, I do not believe that enforcing consistent formatting is a good idea. The configuration included here is highly opinionated, and there is no reason to enforce most of those on users of the MDK. If it is meant to be opt in, as you suggest - then it should be entirely commented out by default, like all the other "opt in" features of the MDK, such as demonstrating how to depend on other mods and the like. I would not be against including a commented-out spotless configuration and instructions for using spotless, but applying a fairly non-idiomatic gradle plugin and a formatting configuration by default - even if the enforcement is opt-in - isn't a good idea.

Drullkus commented 5 months ago

Would you guys be open to using an .editorconfig instead?

While it's an IntelliJ-exclusive feature involving one of their built-in plugins (documentation), adding it is extremely straightforward (implementation example using just 1 file) and adds no complications to the building system.

Beginning devs won't have to worry about fussing with a plugin they didn't ask for, since they'll instead have a file at the project root. I think it will help too that the file is more clear about how developers can change their style preferences for a project, including outright deleting the file if they choose to not enforce any style.

Shadows-of-Fire commented 5 months ago

That would be equivalent to just including the added eclipse formatter xml (both IDEs can import it) and being done with it.

Unfortunately in testing it seemed that enforceCheck false wasn't actually accomplishing anything (the check still runs and fails the build), so including the formatter xml with the plugin setup commented out may be the way to go

Drullkus commented 5 months ago

Right... Just now realizing that this PR is for enforced formatting, which I have to say that I'm not a fan of forcing onto junior devs either. Gradle is already confusing as-is

thedarkcolour commented 4 months ago

Why?

Shadows-of-Fire commented 4 months ago

From local testing enforceCheck false doesn't appear to actually function, so we can reduce this to adding the necessary template lines commented out.

Technici4n commented 4 months ago

IMO: Let's apply our formatting standards to the example code, but let's not add a commented out template.

neoforged-automation[bot] commented 1 month ago

@Shadows-of-Fire, this pull request has conflicts, please resolve them for this PR to move forward.