search

neoforged / FancyModLoader

The fancy mod loader for NeoForged
Other
62 stars 30 forks source link

Add a method to validate the config spec #184

Closed Matyrobbrt closed 2 months ago

Matyrobbrt commented 2 months ago

Add a IConfigSpec#validateSpec method to validate the specification in the context of a mod config, i.e. to check if a value restart type is valid for a given config type.

neoforged-pr-publishing[bot] commented 2 months ago

Last commit published: 8f6339c8d67c4b767395828820a44e5fa66802ca.

PR Publishing ### The artifacts published by this PR: - :package: [`net.neoforged.fancymodloader:earlydisplay:4.0.22-pr-184-validate-config-spec-`](https://github.com/neoforged/FancyModLoader/packages/2204697) - :package: [`net.neoforged.fancymodloader:loader:4.0.22-pr-184-validate-config-spec-`](https://github.com/neoforged/FancyModLoader/packages/2204700) - :package: [`net.neoforged.fancymodloader:junit-fml:4.0.22-pr-184-validate-config-spec-`](https://github.com/neoforged/FancyModLoader/packages/2204699) ### 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 #184' // https://github.com/neoforged/FancyModLoader/pull/184 url 'https://prmaven.neoforged.net/FancyModLoader/pr184' content { includeModule('net.neoforged.fancymodloader', 'earlydisplay') includeModule('net.neoforged.fancymodloader', 'loader') includeModule('net.neoforged.fancymodloader', 'junit-fml') } } } ```
neoforged-compatibility-checks[bot] commented 2 months ago

@Matyrobbrt, this PR introduces breaking changes. Fortunately, this project is currently accepting breaking changes, but if they are not intentional, please revert them. Last checked commit: 8f6339c8d67c4b767395828820a44e5fa66802ca.

loader (:loader)

marchermans commented 2 months ago

What is the context of this change? Why do you want to add this?

Matyrobbrt commented 2 months ago

This PR was made specifically for https://github.com/neoforged/NeoForge/pull/1284 in order to validate if a restart type is compatible with a config type (i.e. GAME restart not being compatible with SERVRE configs)

marchermans commented 2 months ago

The mentioned PR, does not update FML, nor does it use the newly introduced method validateSpec.

Could you elaborate the idea for this change?

Matyrobbrt commented 2 months ago

I'm not sure what I should elaborate more on. It's quite simple, make sure that the specification is valid when used by a specific config type. So that the neo config spec can throw when someone attempts to use a game restart flag for a server config (which doesn't make sense)

marchermans commented 2 months ago

)

Okey..... But this is not used in that PR Do you intend to make another PR to add that? Or what is your plan?

Also I think a GameRestart flag on Server config makes a lot of sense. Why would we disallow that. It indicates that the server needs to be restarted.

Matyrobbrt commented 2 months ago

Okey..... But this is not used in that PR

Yet, I just made this PR yesterday...

Also I think a GameRestart flag on Server config makes a lot of sense. Why would we disallow that. It indicates that the server needs to be restarted.

That's a world restart which is a separate flag that actually works with SP worlds.

marchermans commented 2 months ago

Okey..... But this is not used in that PR

Yet, I just made this PR yesterday...

Also I think a GameRestart flag on Server config makes a lot of sense. Why would we disallow that. It indicates that the server needs to be restarted.

That's a world restart which is a separate flag that actually works with SP worlds.

Isn't a game restart and a server restart on the server the same? So why would seperating this out matter?

TelepathicGrunt commented 2 months ago

Isn't a game restart and a server restart on the server the same? So why would separating this out matter?

I would just treat both flags the same on a server config tbh instead of erroring for one. Erroring will lead to modder confusion as game restart sounds like it should restart server as the server is the game.

Matyrobbrt commented 2 months ago

The problem is that the flags ought to behave the same in all contexts, and they are radically different in a non singleton context (singleplayer). These flags are, if anything, more user facing than modder facing and their intent and effect need to be clear to users.

While for a physical server, game and server restart are the same, for a single player world they're completely different concepts.

marchermans commented 2 months ago

The problem is that the flags ought to behave the same in all contexts, and they are radically different in a non singleton context (singleplayer). These flags are, if anything, more user facing than modder facing and their intent and effect need to be clear to users.

While for a physical server, game and server restart are the same, for a single player world they're completely different concepts.

In what way are they completely different contexts? Both indicate exactly what they are supposed to do, whether in dedicated server, or in integrated server. Just in dedicated server the only way to restart a world is by restarting the game. So I agree with grunt here. I see no reason to differentiate on these flags, and as such no reason to merge this PR.

TelepathicGrunt commented 2 months ago

Change my mind. Blocking game restart for server config to require world restart makes more sense in single player context and not cause users to restart the game when they don’t need to. Just exit and re-enter world

marchermans commented 2 months ago

Change my mind. Blocking game restart for server config to require world restart makes more sense in single player context and not cause users to restart the game when they don’t need to. Just exit and re-enter world

Agreed, but again that discussion is now not on the record here. So it will get lost to time.

However approving.