[1.21.3] Encourage more usage of IConditionBuider #1716
Conversation
by deprecating all shortcut constructors and moving them into `IConditionBuilder`
Last commit published: e2da34f7dfabb3644c92149210097427b2ad9131. PR PublishingThe artifacts published by this PR:
Repository DeclarationIn order to use the artifacts published by the PR, add the following repository to your buildscript: repositories {
maven {
name 'Maven for PR #1716' // https://github.com/neoforged/NeoForge/pull/1716
url 'https://prmaven.neoforged.net/NeoForge/pr1716'
content {
includeModule('net.neoforged', 'testframework')
includeModule('net.neoforged', 'neoforge')
}
}
}MDK installationIn order to setup a MDK using the latest PR version, run the following commands in a terminal. mkdir NeoForge-pr1716
cd NeoForge-pr1716
curl -L https://prmaven.neoforged.net/NeoForge/pr1716/net/neoforged/neoforge/21.3.57-beta-pr-1716-pr-condition-builder/mdk-pr1716.zip -o mdk.zip
jar xf mdk.zip
rm mdk.zip || del mdk.zipTo test a production environment, you can download the installer from here. |
less of a breaking change
| import org.slf4j.Logger; | ||
|
|
||
| public interface DataProvider { | ||
| -public interface DataProvider { |
There was a problem hiding this comment.
Do we really want it in every data provider? Or should we be more selective and only implement it where we know it is usable?
There was a problem hiding this comment.
I remembered seeing this PR (#1315) and thought our condition system supported any data file now, but guess I my assumption on that was wrong as it was closed not merged.
I can go through and find all the providers which directly support conditions, rather than implementing onto DataProvider.
| /** | ||
| * @deprecated Use {@link IConditionBuilder} | ||
| */ | ||
| @Deprecated(forRemoval = true, since = "1.12.3") |
There was a problem hiding this comment.
Good catch, should be fixed now
| @@ -46,8 +45,8 @@ static void conditionalDatapackEntries(final DynamicTest test, final Registratio | |||
| event.getLookupProvider(), | |||
| builder, | |||
| conditions -> { | |||
| conditions.accept(CONDITIONAL_FALSE_DAMAGE_TYPE, FalseCondition.INSTANCE); | |||
| conditions.accept(CONDITIONAL_TRUE_DAMAGE_TYPE, TrueCondition.INSTANCE); | |||
| conditions.accept(CONDITIONAL_FALSE_DAMAGE_TYPE, IConditionBuilder.of().never()); | |||
There was a problem hiding this comment.
Makes me wonder if static methods in the builder would work instead. 🤔
There was a problem hiding this comment.
I was thinking that too, but that would have to wait until 21.4
Since afaik cant make static versions cause the method signature will conflict the the default ones.
| @@ -17,14 +19,32 @@ default ICondition and(ICondition... values) { | |||
| return new AndCondition(List.of(values)); | |||
| } | |||
|
|
|||
| default ICondition FALSE() { | |||
| default ICondition never() { | |||
| // TODO: Maybe also rename class and registry entry to 'never' as well | |||
There was a problem hiding this comment.
This kind of TODO will get forgotten and come back to haunt us in a few years. :P
There was a problem hiding this comment.
Deprecated the old TRUE/FALSE in favor of newly added always/never conditions
This PR aims to encourage modders to make more use of
IConditionBuilderrather than constructing condition instances themselvesthis is done by
DataProviders which support conditions to now implementIConditionBuildermaking all of its helpers freely available to all inheritors.ofmethod allowing users to construct an instance in places where they do not have aDataProvideror can implement it themselves.ICondtionBuilderTRUE/FALSEconditions in favor of the newalways/neverconditions