neoforged / NeoForge

Neo Modding API for Minecraft, based on Forge
https://neoforged.net
Other
1.24k stars 179 forks source link

[1.21.x] Allow changing datagen indent width #1687

Closed Shadows-of-Fire closed 4 days ago

Shadows-of-Fire commented 1 week ago

Pretty straightforward. I wanted to do this, was going to do it with a mixin, and then saw that the location is in a lambda. I didn't want to spin up a coremod for it and deal with the mess that is lambda-mixins, so I figured why not just patch the thing in.

Modders can set the value by calling DataProvider.INDENT_WIDTH.set(...). I'll backport this to 1.21.1 after this PR is complete.

neoforged-pr-publishing[bot] commented 1 week ago

Last commit published: 0b751ef163406e0651c7c8238c409457b2d624b6.

PR Publishing ### The artifacts published by this PR: - :package: [`net.neoforged:testframework:21.3.30-beta-pr-1687-indent-width`](https://github.com/neoforged/NeoForge/packages/2313150) - :package: [`net.neoforged:neoforge:21.3.30-beta-pr-1687-indent-width`](https://github.com/neoforged/NeoForge/packages/2313149) ### Repository Declaration In order to use the artifacts published by the PR, add the following repository to your buildscript: ```gradle repositories { maven { name 'Maven for PR #1687' // https://github.com/neoforged/NeoForge/pull/1687 url 'https://prmaven.neoforged.net/NeoForge/pr1687' content { includeModule('net.neoforged', 'testframework') includeModule('net.neoforged', 'neoforge') } } } ``` ### MDK installation In order to setup a MDK using the latest PR version, run the following commands in a terminal. The script works on both *nix and Windows as long as you have the JDK `bin` folder on the path. The script will clone the MDK in a folder named `NeoForge-pr1687`. On Powershell you will need to remove the `-L` flag from the `curl` invocation. ```sh mkdir NeoForge-pr1687 cd NeoForge-pr1687 curl -L https://prmaven.neoforged.net/NeoForge/pr1687/net/neoforged/neoforge/21.3.30-beta-pr-1687-indent-width/mdk-pr1687.zip -o mdk.zip jar xf mdk.zip rm mdk.zip || del mdk.zip ``` To test a production environment, you can download the installer from [here](https://prmaven.neoforged.net/NeoForge/pr1687/net.neoforged/neoforge/21.3.30-beta-pr-1687-indent-width/neoforge-21.3.30-beta-pr-1687-indent-width-installer.jar).
Gaming32 commented 1 week ago

Since Gson already takes a string for its indent, why not do the same for this?

Shadows-of-Fire commented 1 week ago

I used an atomic int here because the field is in an interface (meaning it has to be final) and I wanted to keep anything centralized.

Gaming32 commented 1 week ago

So why not AtomicReference<String>?

Shadows-of-Fire commented 1 week ago

I don't think anyone needs to be using non-spaces as the indentation. There's the possibility someone wants to use tabs, but in the name of whole ecosystem consistency, use of spaces everywhere is likely to shake out as the best case. Additionally, INTENT_WIDTH.set(4) is less error prone than INDENT.set(" "). A reader can immediately know the number of spaces when reading the code without having to count them.

Gaming32 commented 1 week ago

I don't think bringing up consistency is fair, as this PR generally reduces that, but the readability point is fair.

neoforged-releases[bot] commented 4 days ago

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

SquidDev commented 4 days ago

Not sure if helpful, but if people do want to use tabs (or format the JSON in another way), you can wrap the CachedOutput instance to reformat the files before writing them. It's a bit of a hack, but saves mixins in awkward places :).