-
-
Notifications
You must be signed in to change notification settings - Fork 59
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
base: 1.21
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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.
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. |
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 |
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 |
If anything, this should be opt-in, with the plugin and config block commented out with explanations of the pros and cons. |
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. |
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).
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 |
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 |
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. |
Would you guys be open to using an 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. |
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 |
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 |
Why? |
From local testing |
IMO: Let's apply our formatting standards to the example code, but let's not add a commented out template. |
@Shadows-of-Fire, this pull request has conflicts, please resolve them for this PR to move forward. |
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
).