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 spotless to the MDK #52

Open
wants to merge 1 commit into
base: 1.21
Choose a base branch
from

Conversation

Shadows-of-Fire
Copy link

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).

@Shadows-of-Fire Shadows-of-Fire added the enhancement New feature or request label Apr 28, 2024
@Shadows-of-Fire Shadows-of-Fire self-assigned this Apr 28, 2024
Copy link
Member

@Technici4n Technici4n left a comment

Choose a reason for hiding this comment

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

I think that is a good idea. Auto formatters are good practice and people can opt out if they don't want that.

@TelepathicGrunt
Copy link

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
Copy link

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
Copy link

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
Copy link

dhyces commented Apr 28, 2024

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

@sciwhiz12
Copy link
Member

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
Copy link
Author

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
Copy link

lukebemish commented Apr 28, 2024

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
Copy link

lukebemish commented Apr 28, 2024

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
Copy link

Drullkus commented Apr 29, 2024

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
Copy link
Author

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
Copy link

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
Copy link

Why?

@Shadows-of-Fire
Copy link
Author

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
Copy link
Member

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

@neoforged-automation
Copy link

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

@neoforged-automation neoforged-automation bot added the needs rebase This Pull Request needs to be rebased before being merged label Aug 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs rebase This Pull Request needs to be rebased before being merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants