-
-
Notifications
You must be signed in to change notification settings - Fork 416
Standardize Module usage + add child modules #8346
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
base: dev/feature
Are you sure you want to change the base?
Conversation
| private static final boolean BREWING_START_EVENT_1_21 = Skript.methodExists(BrewingStartEvent.class, "setBrewingTime", int.class); | ||
|
|
||
| public static void register(SyntaxRegistry registry) { | ||
| public static void register(SyntaxRegistry registry, AddonModule.ModuleOrigin origin) { |
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.
| public static void register(SyntaxRegistry registry, AddonModule.ModuleOrigin origin) { | |
| public static void register(SyntaxRegistry registry, ModuleOrigin origin) { |
| Skript.registerCondition(CondWasIndirect.class, ConditionType.PROPERTY, | ||
| "%damagesources% (was|were) ([:in]directly caused|caused [:in]directly)", | ||
| "%damagesources% (was not|wasn't|were not|weren't) ([:in]directly caused|caused [:in]directly)" | ||
| public static void register(SyntaxRegistry registry, AddonModule.ModuleOrigin origin) { |
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.
| public static void register(SyntaxRegistry registry, AddonModule.ModuleOrigin origin) { | |
| public static void register(SyntaxRegistry registry, ModuleOrigin origin) { |
| public static void register(SyntaxRegistry registry, ModuleOrigin origin) { | ||
| registry.register( | ||
| SyntaxRegistry.EXPRESSION, | ||
| infoBuilder(ExprCreatedDamageSource.class, DamageSource.class,"created damage source") |
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.
| infoBuilder(ExprCreatedDamageSource.class, DamageSource.class,"created damage source") | |
| infoBuilder(ExprCreatedDamageSource.class, DamageSource.class, "created damage source") |
| public static void register(SyntaxRegistry registry, ModuleOrigin origin) { | ||
| registry.register( | ||
| SyntaxRegistry.EXPRESSION, | ||
| infoBuilder(ExprDamageType.class, DamageType.class,"damage type", "damagesources", true) |
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.
| infoBuilder(ExprDamageType.class, DamageType.class,"damage type", "damagesources", true) | |
| infoBuilder(ExprDamageType.class, DamageType.class, "damage type", "damagesources", true) |
| public static void register(SyntaxRegistry registry, AddonModule.ModuleOrigin origin) { | ||
| registry.register( | ||
| SyntaxRegistry.EXPRESSION, | ||
| infoBuilder(ExprDirectEntity.class, Entity.class,"direct entity", "damagesources", true) |
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.
| infoBuilder(ExprDirectEntity.class, Entity.class,"direct entity", "damagesources", true) | |
| infoBuilder(ExprDirectEntity.class, Entity.class, "direct entity", "damagesources", true) |
| public static void register(SyntaxRegistry registry, ModuleOrigin origin) { | ||
| registry.register( | ||
| SyntaxRegistry.EXPRESSION, | ||
| infoBuilder(ExprFoodExhaustion.class, Float.class,"food exhaustion", "damagesources", true) |
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.
| infoBuilder(ExprFoodExhaustion.class, Float.class,"food exhaustion", "damagesources", true) | |
| infoBuilder(ExprFoodExhaustion.class, Float.class, "food exhaustion", "damagesources", true) |
|
|
||
| static { | ||
| registerDefault(ExprDirectEntity.class, Entity.class, "direct entity", "damagesources"); | ||
| public static void register(SyntaxRegistry registry, AddonModule.ModuleOrigin origin) { |
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.
| public static void register(SyntaxRegistry registry, AddonModule.ModuleOrigin origin) { | |
| public static void register(SyntaxRegistry registry, ModuleOrigin origin) { |
| public static void register(SyntaxRegistry registry, ModuleOrigin origin) { | ||
| registry.register( | ||
| SyntaxRegistry.EXPRESSION, | ||
| infoBuilder(ExprSourceLocation.class, Location.class,"source location", "damagesources", true) |
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.
| infoBuilder(ExprSourceLocation.class, Location.class,"source location", "damagesources", true) | |
| infoBuilder(ExprSourceLocation.class, Location.class, "source location", "damagesources", true) |
| Skript.registerCondition(CondFishingLure.class, | ||
| "lure enchantment bonus is (applied|active)", | ||
| "lure enchantment bonus is(n't| not) (applied|active)"); | ||
| public static void register(SyntaxRegistry registry, Origin origin) { |
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.
Shouldn't these take in ModuleOrigin?
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.
no the others should be taking in Origin
APickledWalrus
left a comment
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.
Looks pretty good overall, just what I found from a Quick Look
| * @return The name of the module represented by this origin. | ||
| * @return The names of the modules represented by this origin. | ||
| */ | ||
| String moduleName(); |
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.
A deprecated version of this method should be retained. It can just join moduleNames with a space or something.
| * @param consumers The consumers that will register syntax to the registry. | ||
| */ | ||
| @SafeVarargs | ||
| static void register(SyntaxRegistry registry, ModuleOrigin origin, BiConsumer<SyntaxRegistry, ModuleOrigin>... consumers) { |
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 we are fine to just allow Origin here
| @Override | ||
| public ModuleOrigin origin(SkriptAddon addon) { | ||
| return ChildAddonModule.origin(addon, parentModule, name()); | ||
| } |
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.
| } | |
| } | |
| * @param moduleName The name of the providing module. | ||
| */ | ||
| public ChildModuleOriginImpl(SkriptAddon addon, AddonModule parentModule, String moduleName) { | ||
| this.addon = addon; |
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.
This should be an unmodifiable view. I need to fix this for the regular ModuleOrigin too.
| public static void register(SyntaxRegistry registry, ModuleOrigin origin) { | ||
| registry.register( | ||
| SyntaxRegistry.EXPRESSION, | ||
| infoBuilder(ExprCausingEntity.class, Entity.class,"(causing|responsible) entity", "damagesources", true) |
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.
| infoBuilder(ExprCausingEntity.class, Entity.class,"(causing|responsible) entity", "damagesources", true) | |
| infoBuilder(ExprCausingEntity.class, Entity.class, "(causing|responsible) entity", "damagesources", true) |
| "fish[ing] bit(e|ing) [wait] time"); | ||
| public static void register(SyntaxRegistry registry, Origin origin) { | ||
| registry.register(SyntaxRegistry.EXPRESSION, | ||
| DefaultSyntaxInfos.Expression.builder(ExprFishingBiteTime.class, Timespan.class) |
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.
| DefaultSyntaxInfos.Expression.builder(ExprFishingBiteTime.class, Timespan.class) | |
| SyntaxInfo.Expression.builder(ExprFishingBiteTime.class, Timespan.class) |
And same with priority here too
| "hook[ed] entity"); | ||
| public static void register(SyntaxRegistry registry, Origin origin) { | ||
| registry.register(SyntaxRegistry.EXPRESSION, | ||
| DefaultSyntaxInfos.Expression.builder(ExprFishingHookEntity.class, Entity.class) |
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.
| DefaultSyntaxInfos.Expression.builder(ExprFishingHookEntity.class, Entity.class) | |
| SyntaxInfo.Expression.builder(ExprFishingHookEntity.class, Entity.class) |
And same with priority here too
| "(min:min[imum]|max[imum]) fish[ing] wait[ing] time"); | ||
| public static void register(SyntaxRegistry registry, Origin origin) { | ||
| registry.register(SyntaxRegistry.EXPRESSION, | ||
| DefaultSyntaxInfos.Expression.builder(ExprFishingWaitTime.class, Timespan.class) |
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.
| DefaultSyntaxInfos.Expression.builder(ExprFishingWaitTime.class, Timespan.class) | |
| SyntaxInfo.Expression.builder(ExprFishingWaitTime.class, Timespan.class) |
And same with priority here too
| public static void register(SyntaxRegistry registry, ModuleOrigin origin) { | ||
| registry.register( | ||
| SyntaxRegistry.EXPRESSION, | ||
| DefaultSyntaxInfos.Expression.builder(ExprSecDamageSource.class, DamageSource.class) |
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.
| DefaultSyntaxInfos.Expression.builder(ExprSecDamageSource.class, DamageSource.class) | |
| SyntaxInfo.Expression.builder(ExprSecDamageSource.class, DamageSource.class) |
| /** | ||
| * @return A name representing this module to be using in the {@link ModuleOrigin}. e.g. "discord", "nbt", "particles"... | ||
| */ | ||
| String name(); |
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.
We'll ideally want to avoid a breaking change here. Maybe we can provide a default implementation for now that returns the addon name or something? Not sure how feasible it would be, but we could also log a warning that the addon needs to implement it.
Problem
Skript contains a lot of modules now, some properly using the registry api, some not, and all in a very flat structure.
It would be better to have all the bukkit modules under a single bukkit module parent, and it would be nice to standardize how modules are made in Skript.
Solution
Refactors all existing modules to fit the same standard for addon modules, creating a required
name()method for creating origins, using origins properly, and creating ChildAddonModules. These act nearly identically to AddonModules, but contain a reference to a parent module for use during loading or creating origins.Module origins can now have multiple providing modules, with the most specific first. Child modules will automatically handle this, providing origins that may have a list of modules like
[interactions, entities, bukkit].Testing Completed
No new tests, just existing ones.
Supporting Information
Completes: none
Related: none
AI assistance: none