Additional module standardization (#8516)

Author: APickledWalrus

code-conventions.md

@@ -141,7 +141,25 @@ but this is not strictly required:
   String string = "example string " +
         "with more to add";
   ```
-  
+
+#### Module Organization
+
+When creating modules, they should be organized in the following manner:
+```text
+module
+|- elements
+|  |- expressions
+|  |  |- ExprX.java
+|  |- effects
+|  |  |- EffY.java
+|- submodule
+|  |- elements
+|  |- Submodule.java
+|- Module.java
+```
+
+#### Syntax Class Organization
+
 * When extending one of following classes: SimpleExpression, SimplePropertyExpression, Effect, Condition...
   - Put overridden methods in order
   - Put static registration before all methods
@@ -153,7 +171,6 @@ but this is not strictly required:
   - Section: init -> walk -> toString
   - Structure: init -> (preLoad) -> load -> (postLoad) -> unload -> (postUnload) -> (getPriority) -> toString
 
-
 ### Naming
 * Class names are written in `UpperCamelCase`
   - The file name should match its primary class name (e.g. `MyClass` goes in `MyClass.java`.)

src/main/java/ch/njol/skript/Skript.java

@@ -60,6 +60,7 @@
 import org.junit.runner.JUnitCore;
 import org.junit.runner.Result;
 import org.junit.runner.notification.Failure;
+import org.skriptlang.skript.addon.AddonModule;
 import org.skriptlang.skript.bukkit.BukkitModule;
 import org.skriptlang.skript.bukkit.SkriptMetrics;
 import org.skriptlang.skript.bukkit.lang.eventvalue.EventValueRegistry;
@@ -479,8 +480,17 @@ public void onEnable() {
 		//noinspection removal
 		EventValues.setEventValueRegistry(eventValueRegistry);
 
-		// Load classes which are always safe to use
-		new JavaClasses(); // These may be needed in configuration
+		// TODO this upcoming portion is a bad circular dependency
+		// some modules depend on the config
+		// the config depends on some modules (for the types they register)
+
+		// load classes
+		new JavaClasses();
+		new SkriptClasses();
+		new BukkitClasses();
+
+		// load the config
+		SkriptConfig.load();
 
 		// Check server software, Minecraft version, etc.
 		if (!checkServerPlatform()) {
@@ -489,15 +499,6 @@ public void onEnable() {
 			return;
 		}
 
-		// And then not-so-safe classes
-		Throwable classLoadError = null;
-		try {
-			new SkriptClasses();
-			new BukkitClasses();
-		} catch (Throwable e) {
-			classLoadError = e;
-		}
-
 		// Warn about pausing
 		if (Skript.methodExists(Server.class, "getPauseWhenEmptyTime")) {
 			int pauseThreshold = getServer().getPauseWhenEmptyTime();
@@ -508,14 +509,8 @@ public void onEnable() {
 			}
 		}
 
-
-		// Config must be loaded after Java and Skript classes are parseable
-		// ... but also before platform check, because there is a config option to ignore some errors
-		SkriptConfig.load();
-
 		// Register the runtime error refresh after loading, so we can do the first instantiation manually.
 		SkriptConfig.eventRegistry().register(SkriptConfig.ReloadEvent.class, RuntimeErrorManager::refresh);
-
 		// init runtime error manager and add bukkit consumer.
 		RuntimeErrorManager.refresh();
 		getRuntimeErrorManager().addConsumer(new BukkitRuntimeErrorConsumer());
@@ -532,35 +527,48 @@ public void onEnable() {
 			updater.updateCheck(console);
 		}
 
-		// If loading can continue (platform ok), check for potentially thrown error
-		if (classLoadError != null) {
-			exception(classLoadError);
-			setEnabled(false);
-			return;
-		}
-
 		PluginCommand skriptCommand = getCommand("skript");
 		assert skriptCommand != null; // It is defined, unless build is corrupted or something like that
 		skriptCommand.setExecutor(new SkriptCommand());
 		skriptCommand.setTabCompleter(new SkriptCommandTabCompleter());
 
-		// Load Bukkit stuff. It is done after platform check, because something might be missing!
-		BukkitEventValues.register(eventValueRegistry);
+		final AddonModule legacyModule = new AddonModule() {
+
+			@Override
+			public void init(org.skriptlang.skript.addon.SkriptAddon addon) {
+				new DefaultComparators();
+				new DefaultConverters();
 
-		new DefaultComparators();
-		new DefaultConverters();
-		new DefaultFunctions();
-		new DefaultOperations();
+				// legacy listeners
+				//noinspection removal
+				ChatMessages.registerListeners();
+			}
+
+			@Override
+			public void load(org.skriptlang.skript.addon.SkriptAddon addon) {
+				try {
+					//noinspection removal
+					getAddonInstance().loadClasses("ch.njol.skript",
+						"conditions", "effects", "events", "expressions", "entity", "literals", "sections", "structures");
+				} catch (IOException e) {
+					throw new RuntimeException(e);
+				}
+				BukkitEventValues.register(eventValueRegistry);
+				new DefaultFunctions();
+				new DefaultOperations();
+			}
+
+			@Override
+			public String name() {
+				return "legacy";
+			}
 
-		ChatMessages.registerListeners();
+		};
 
 		try {
-			getAddonInstance().loadClasses("ch.njol.skript",
-				"conditions", "effects", "events", "expressions", "entity", "literals", "sections", "structures");
-			getAddonInstance().loadClasses("org.skriptlang.skript.bukkit", "misc");
-			skript.loadModules(new CommonModule(), new BukkitModule());
+			skript.loadModules(legacyModule, new CommonModule(), new BukkitModule());
 		} catch (final Exception e) {
-			exception(e, "Could not load required .class files: " + e.getLocalizedMessage());
+			exception(e, "Failed to load one of Skript's modules: " + e.getLocalizedMessage());
 			setEnabled(false);
 			return;
 		}

src/main/java/ch/njol/skript/classes/data/BukkitClasses.java

@@ -49,8 +49,7 @@
 import org.bukkit.metadata.Metadatable;
 import org.bukkit.util.CachedServerIcon;
 import org.jetbrains.annotations.Nullable;
-import org.skriptlang.skript.bukkit.base.types.*;
-import org.skriptlang.skript.bukkit.base.types.EntityClassInfo.EntityChanger;
+import org.skriptlang.skript.bukkit.types.EntityClassInfo.EntityChanger;
 import org.skriptlang.skript.lang.properties.Property;
 import org.skriptlang.skript.lang.properties.handlers.base.ExpressionPropertyHandler;
 
@@ -66,8 +65,6 @@ public class BukkitClasses {
 	public BukkitClasses() {}
 
 	static {
-		Classes.registerClass(new EntityClassInfo());
-
 		Classes.registerClass(new ClassInfo<>(LivingEntity.class, "livingentity")
 				.user("living ?entit(y|ies)")
 				.name("Living Entity")
@@ -91,8 +88,6 @@ public BukkitClasses() {}
 				.defaultExpression(new EventValueExpression<>(Projectile.class))
 				.changer(DefaultChangers.nonLivingEntityChanger));
 
-		Classes.registerClass(new BlockClassInfo());
-
 		Classes.registerClass(new ClassInfo<>(BlockData.class, "blockdata")
 				.user("block ?datas?")
 				.name("Block Data")
@@ -154,9 +149,6 @@ protected boolean canBeInstantiated() {
 					}
 				}).cloner(BlockData::clone));
 
-		Classes.registerClass(new LocationClassInfo());
-		Classes.registerClass(new VectorClassInfo());
-
 		Classes.registerClass(new ClassInfo<>(World.class, "world")
 				.user("worlds?")
 				.name("World")
@@ -231,8 +223,6 @@ public boolean mustSyncDeserialization() {
 					ExpressionPropertyHandler.of(World::getName, String.class)
 				));
 
-		Classes.registerClass(new InventoryClassInfo());
-
 		Classes.registerClass(new EnumClassInfo<>(InventoryAction.class, "inventoryaction", "inventory actions")
 				.user("inventory ?actions?")
 				.name("Inventory Action")
@@ -255,10 +245,6 @@ public boolean mustSyncDeserialization() {
 				.examples("")
 				.since("2.2-dev32"));
 
-		Classes.registerClass(new PlayerClassInfo());
-
-		Classes.registerClass(new OfflinePlayerClassInfo());
-
 		Classes.registerClass(new ClassInfo<>(CommandSender.class, "commandsender")
 				.user("((commands?)? ?)?(sender|executor)s?")
 				.name("Command Sender")
@@ -300,8 +286,6 @@ public String toVariableNameString(final CommandSender s) {
 					ExpressionPropertyHandler.of(CommandSender::name, Component.class)
 				));
 
-		Classes.registerClass(new NameableClassInfo());
-
 		Classes.registerClass(new ClassInfo<>(InventoryHolder.class, "inventoryholder")
 				.name(ClassInfo.NO_DOC)
 				.defaultExpression(new EventValueExpression<>(InventoryHolder.class))
@@ -341,8 +325,6 @@ public String toVariableNameString(InventoryHolder holder) {
 						"set the player argument's game mode to creative")
 				.since("1.0"));
 
-		Classes.registerClass(new ItemStackClassInfo());
-
 		Classes.registerClass(new ClassInfo<>(Item.class, "itementity")
 				.name(ClassInfo.NO_DOC)
 				.since("2.0")

src/main/java/ch/njol/skript/classes/data/DefaultChangers.java

@@ -9,10 +9,10 @@
 import org.bukkit.inventory.Inventory;
 import org.bukkit.inventory.ItemStack;
 import org.jetbrains.annotations.Nullable;
-import org.skriptlang.skript.bukkit.base.types.BlockClassInfo;
-import org.skriptlang.skript.bukkit.base.types.EntityClassInfo;
-import org.skriptlang.skript.bukkit.base.types.InventoryClassInfo;
-import org.skriptlang.skript.bukkit.base.types.PlayerClassInfo;
+import org.skriptlang.skript.bukkit.types.BlockClassInfo;
+import org.skriptlang.skript.bukkit.types.EntityClassInfo;
+import org.skriptlang.skript.bukkit.types.InventoryClassInfo;
+import org.skriptlang.skript.bukkit.types.PlayerClassInfo;
 
 /**
  * @author Peter Güttinger

src/main/java/ch/njol/skript/classes/data/JavaClasses.java

@@ -16,7 +16,6 @@
 import ch.njol.yggdrasil.Fields;
 import org.jetbrains.annotations.Contract;
 import org.jetbrains.annotations.Nullable;
-import org.skriptlang.skript.common.types.QuaternionClassInfo;
 import org.skriptlang.skript.lang.properties.Property;
 import org.skriptlang.skript.lang.properties.handlers.ContainsHandler;
 import org.skriptlang.skript.lang.properties.handlers.base.ConditionPropertyHandler;
@@ -324,9 +323,6 @@ public Class<? extends String>[] elementTypes() {
 				Skript.instance(),
 				ConditionPropertyHandler.of(String::isEmpty)));
 
-		// joml type - for display entities
-		Classes.registerClass(new QuaternionClassInfo());
-
 
 		Classes.registerClass(new ClassInfo<>(UUID.class, "uuid")
 			.user("uuids?")

src/main/java/ch/njol/skript/classes/data/SkriptClasses.java

@@ -21,10 +21,6 @@
 import ch.njol.yggdrasil.Fields;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
-import org.skriptlang.skript.bukkit.base.types.ItemTypeClassInfo;
-import org.skriptlang.skript.bukkit.base.types.SlotClassInfo;
-import org.skriptlang.skript.common.types.QueueClassInfo;
-import org.skriptlang.skript.common.types.ScriptClassInfo;
 import org.skriptlang.skript.lang.properties.Property;
 import org.skriptlang.skript.lang.properties.handlers.ContainsHandler;
 import org.skriptlang.skript.lang.properties.handlers.TypedValueHandler;
@@ -146,8 +142,6 @@ public String toVariableNameString(final WeatherType o) {
 				})
 				.serializer(new EnumSerializer<>(WeatherType.class)));
 
-		Classes.registerClass(new ItemTypeClassInfo());
-
 		Classes.registerClass(new ClassInfo<>(Time.class, "time")
 				.user("times?")
 				.name("Time")
@@ -349,8 +343,6 @@ public String toVariableNameString(final Direction o) {
 				})
 				.serializer(new YggdrasilSerializer<>()));
 
-		Classes.registerClass(new SlotClassInfo());
-
 		Classes.registerClass(new ClassInfo<>(Color.class, "color")
 				.user("colou?rs?")
 				.name("Color")
@@ -483,8 +475,6 @@ public String toVariableNameString(final Experience xp) {
 				.serializer(new YggdrasilSerializer<GameruleValue>())
 		);
 
-		Classes.registerClass(new QueueClassInfo());
-
 		Classes.registerClass(new ClassInfo<>(Config.class, "config")
 			.user("configs?")
 			.name("Config")
@@ -567,8 +557,6 @@ public String toVariableNameString(Node node) {
 					}
 				}));
 
-		Classes.registerClass(new ScriptClassInfo());
-
 		Classes.registerClass(new ClassInfo<>(Executable.class, "executable")
 			.user("executables?")
 			.name("Executable")

src/main/java/ch/njol/skript/conditions/CondContains.java

@@ -22,7 +22,7 @@
 import org.bukkit.inventory.Inventory;
 import org.bukkit.inventory.ItemStack;
 import org.jetbrains.annotations.Nullable;
-import org.skriptlang.skript.common.properties.conditions.PropCondContains;
+import org.skriptlang.skript.common.properties.elements.conditions.PropCondContains;
 import org.skriptlang.skript.lang.comparator.Comparators;
 import org.skriptlang.skript.lang.comparator.Relation;
 import org.skriptlang.skript.lang.converter.Converters;

src/main/java/ch/njol/skript/conditions/CondIsEmpty.java

@@ -11,7 +11,7 @@
 import org.bukkit.Material;
 import org.bukkit.inventory.Inventory;
 import org.bukkit.inventory.ItemStack;
-import org.skriptlang.skript.common.properties.conditions.PropCondIsEmpty;
+import org.skriptlang.skript.common.properties.elements.conditions.PropCondIsEmpty;
 import org.skriptlang.skript.lang.util.SkriptQueue;
 
 /**

src/main/java/ch/njol/skript/expressions/ExprAmount.java

@@ -18,7 +18,7 @@
 import ch.njol.util.coll.CollectionUtils;
 import org.bukkit.event.Event;
 import org.jetbrains.annotations.Nullable;
-import org.skriptlang.skript.common.properties.expressions.PropExprAmount;
+import org.skriptlang.skript.common.properties.elements.expressions.PropExprAmount;
 
 /**
  * @deprecated This is being removed in favor of {@link PropExprAmount}

src/main/java/ch/njol/skript/expressions/ExprName.java

@@ -31,7 +31,7 @@
 import org.bukkit.inventory.Inventory;
 import org.bukkit.inventory.InventoryView;
 import org.jetbrains.annotations.Nullable;
-import org.skriptlang.skript.common.properties.expressions.PropExprName;
+import org.skriptlang.skript.common.properties.elements.expressions.PropExprName;
 import org.skriptlang.skript.lang.script.Script;
 
 import java.util.ArrayList;