Skip to content
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

Add Module System #4573

Closed

Conversation

APickledWalrus
Copy link
Member

Description

The purpose of this PR is to start discussion on implementing a new package / syntax organization system for Skript. I created this as a PR as I wanted to be able to show an idea I had.

The actual proposal in this PR is to implement a new Module class. All SkriptAddon's would have one or more modules that contain syntax, classinfos, converters, and more. Skript itself would use this module system to reorganize its syntax. Here's an example:

  • org.skriptlang.skript.potion
    • PotionModule (loads syntax, classinfos, etc.)
    • elements (subpackage)
      • ExprPotionEffects
      • EffPoison
      • etc.

What this PR changes:

  • Skript
    • Loads any modules in new package (org.skriptlang.skript)
    • Now loads hook classes (ch.njol.skript.hooks) using updated SkriptAddon load methods
  • SkriptAddon
    • Cleaned up a lot of stuff
    • Implemented new, more detailed loading methods (gives greater control over how the search is handled and how classes are loaded)
    • loadClasses method can no longer throw an IOException - This change is up for debate, but I personally don't see a reason to not handle it internally (it is unlikely to ever happen anyways) - however, it it were to happen, addons no longer have a way to handle it which could be a downside
    • Added a loadModules method
  • Module (A module is a part of a SkriptAddon containing related syntax, classinfos, converters, etc.)
    • register(SkriptAddon) method
      • Called when this module is loaded, passing the SkriptAddon responsible for loading it as a parameter.
    • loadSyntax methods
      • A shortcut to load syntax elements from this module
        • For example, instead of writing addon.loadClasses("org.skriptlang.skript.potion.elements") you could write loadSyntax(addon)
      • I am unsure if it is a good idea for Skript to be calling loadClasses so frequently. Perhaps it would be better off if syntax was "manually" initialized via new MySyntaxElement()

Please share any questions, feedback or ideas! This proposal is certainly not perfect, so I am looking for any ideas you may have :)


Target Minecraft Versions: N/A
Requirements: None
Related Issues:

@APickledWalrus APickledWalrus added feature Pull request adding a new feature. up for debate When the decision is yet to be debated on the issue in question labels Feb 3, 2022
@bluelhf
Copy link
Contributor

bluelhf commented Feb 4, 2022

On the topic of modules, are there plans to modularise Skript further using JPMS?

@Moderocky
Copy link
Member

Moderocky commented Feb 4, 2022

JPMS

No, this is incompatible with Spigot/CB.

@bluelhf
Copy link
Contributor

bluelhf commented Feb 4, 2022

Ah, of course

@Moderocky
Copy link
Member

Concerning this, we could revive Shane's old suggestion of allowing addons to be registered directly to Skript (as Modules) rather than as Bukkit plugins. Addons like skript-yaml, etc. don't need to be plugins and could save resources by just being our modules.

@@ -518,42 +513,23 @@ public void onEnable() {
@Override
public void run() {
assert Bukkit.getWorlds().get(0).getFullTime() == tick;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this actually for? It's a test-only action but I can't see a comment explaining it.

getAddonInstance().loadClasses(c -> {
if (Hook.class.isAssignableFrom(c) && !c.isInterface() && !Modifier.isAbstract(c.getModifiers())) {
try {
c.getDeclaredConstructor().newInstance();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to move away from using newInstance here - it's the slowest and least-reliable way of creating instances.


try (final JarFile jar = new JarFile(file)) {
boolean hasWithClass = withClass != null;
for (JarEntry e : new EnumerationIterable<>(jar.entries())) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been exploring a more efficient way of getting all classes within a package, will let you know on discord if it's a viable alternative.

return loadClasses(c -> {
if (Module.class.isAssignableFrom(c) && !c.isInterface() && !Modifier.isAbstract(c.getModifiers())) {
try {
((Module) c.getConstructor().newInstance()).register(this);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, we may be able to find a better option for this.

@APickledWalrus
Copy link
Member Author

Concerning this, we could revive Shane's old suggestion of allowing addons to be registered directly to Skript (as Modules) rather than as Bukkit plugins. Addons like skript-yaml, etc. don't need to be plugins and could save resources by just being our modules.

This a good idea but can likely be done in another PR as it requires removing SkriptAddon's dependency on JavaPlugin.

@APickledWalrus
Copy link
Member Author

I've implemented a cache for the jar entries so that the loadClasses method is more performant when being called multiple times. Every SkriptAddon has a cache, and all caches are cleared when Skript stops accepting registrations.

@APickledWalrus APickledWalrus added the 2.7 Targeting a 2.7.X version release label Mar 23, 2022
@APickledWalrus APickledWalrus marked this pull request as draft June 3, 2022 18:40
@APickledWalrus APickledWalrus marked this pull request as ready for review June 3, 2022 19:37
@TPGamesNL TPGamesNL self-requested a review August 5, 2022 12:43
src/main/java/ch/njol/skript/SkriptAddon.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/SkriptAddon.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/SkriptAddon.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/SkriptAddon.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/SkriptAddon.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/SkriptAddon.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/SkriptAddon.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/SkriptAddon.java Outdated Show resolved Hide resolved
@APickledWalrus APickledWalrus mentioned this pull request Sep 28, 2022
@APickledWalrus
Copy link
Member Author

APickledWalrus commented Dec 11, 2022

My most recent commit makes a large change to Skript's general organization. I'm proposing two new main packages: org.skriptlang.skript and org.skriptlang.skriptbukkit

In the future, skriptbukkit would contain all of the plugin related stuff whereas skript would contain the base language + any default syntax (ex: math). I'm proposing that default skript syntax would go in syntax subpackage as well.

I'd certainly like to hear thoughts on this :)

Forgot to update for Structure API
@Moderocky
Copy link
Member

Moderocky commented Dec 12, 2022

org.skriptlang.skript and org.skriptlang.skriptbukkit

Packages should reference the dependency ID so that it is clear where code comes from.
For example org.skriptlang.skript.maths tells us it's from skriptlang.org, it's from the skript dependency and it's about maths.
If the Bukkit parts are going to end up in their own dependency then skriptbukkit is okay, but if they're going to be part of skript they ought to go within the skript package.

It's not guaranteed
and formatting improvements
"common" and "bukkit"
@APickledWalrus
Copy link
Member Author

I've decided to move syntax into two primary locations within org.skriptlang.skript:

  • common (this is for default syntax like math)
  • bukkit (this is for bukkit specific syntax)

@TheLimeGlass
Copy link
Collaborator

Don't you think this should target 2.8? We should have a full package change to com.skriptlang in a major version, not just a module system, but the planned package naming all done in one version.

@APickledWalrus
Copy link
Member Author

Don't you think this should target 2.8? We should have a full package change to com.skriptlang in a major version, not just a module system, but the planned package naming all done in one version.

We can target this for 2.8 now yes. I did not plan to move all syntax with a simple rename. It should happen over time through targeted PRs (like my potions rework) to ensure that all syntax can be updated to current standards :)

@APickledWalrus APickledWalrus added 2.8 Targeting a 2.8.X version release and removed 2.7 Targeting a 2.7.X version release labels Jan 26, 2023
Comment on lines +187 to +206
/**
* Loads all module classes found in the package search.
* @param basePackage The base package to start searching in (e.g. 'ch.njol.skript').
* @param subPackages Specific subpackages to search in (e.g. 'conditions').
* If no subpackages are provided, all subpackages will be searched.
* Note that the search will go no further than the first layer of subpackages.
* @return This SkriptAddon.
*/
@SuppressWarnings("ThrowableNotThrown")
public SkriptAddon loadModules(String basePackage, String... subPackages) {
return loadClasses(basePackage, false, false, c -> {
if (Module.class.isAssignableFrom(c) && !c.isInterface() && !Modifier.isAbstract(c.getModifiers())) {
try {
((Module) c.getConstructor().newInstance()).register(this);
} catch (Exception e) {
Skript.exception(e, "Failed to load module " + c);
}
}
}, subPackages);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer an alternative to this:
a) Code generation of module registration
b) Manual module registration

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the addon discovery thing we talked about on Discord will become mainstream, a modules: ["org.example.addon.math.MathModule", ...] field can be added to addon.json.

@APickledWalrus APickledWalrus changed the base branch from master to dev/feature September 16, 2023 18:17
@APickledWalrus
Copy link
Member Author

Closing as I am planning to reintegrate this system in a more meaningful manner in #5331

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.8 Targeting a 2.8.X version release feature Pull request adding a new feature. up for debate When the decision is yet to be debated on the issue in question
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants