Registry rework #5331
Registry rework #5331
Conversation
AyhamAl-Ali
added
enhancement
There was a problem hiding this comment.
Really great work!!
I've left a decent number of comments, but most are just formatting suggestions that can be batch added if you agree with them :)
I don't immediately have any major concerns. My "biggest" worry would be the inclusion of Event stuff in the default classes - but this is easy fixable (see my comments for more details on this)
Feel free to respond back to my comments here directly or we can discuss on Discord :)
src/main/java/org/skriptlang/skript/registry/SyntaxInfoImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/org/skriptlang/skript/registry/SyntaxInfoImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/org/skriptlang/skript/registry/SyntaxInfoImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/org/skriptlang/skript/registry/SyntaxInfoImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/org/skriptlang/skript/registry/SyntaxInfoImpl.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Patrick Miller <[email protected]>
Co-authored-by: Patrick Miller <[email protected]>
kiip1
requested review from
APickledWalrus
and removed request for
TheLimeGlass
|
You have to be double tabbing on new line method builds and messages. |
# Conflicts: # src/main/java/ch/njol/skript/Skript.java
| */ | ||
| @ApiStatus.Experimental | ||
| @ApiStatus.NonExtendable | ||
| public interface Skript { |
There was a problem hiding this comment.
I am a little unsure about this design. I might prefer something where we offer a generic Skript implementation to developers but also allow it to be customized. We would keep this interface, but also provide some sort of SimpleSkript or GenericSkript implementation that can be used (e.g. a minor rework of SkriptImpl).
This would also allow us to remove the static instance() method here in favor of the developers creating a Skript object themselves (for us, this would be done in the Skript JavaPlugin class). It could be something as simple as SimpleSkript.create()
There was a problem hiding this comment.
I added Skript#setInstance so Skript#instance will still return the current Skript instance (for internal usage). Design might be possible without Skript#instance but it'd mean passing the instance around everywhere.
There was a problem hiding this comment.
IMO a Skript instance should be creatable by the user (e.g. you could run multiple Skript instances at once)
We could do something like Skript.create (keep a default implementation hidden) or do the SimpleSkript method I proposed
| /** | ||
| * Used for sorting in ascending order. | ||
| */ | ||
| int priority(); |
There was a problem hiding this comment.
I wonder if this should use the same Priority class as present in Structure (maybe it could be moved out)? That way a consistent ordering system is used too (lowest numbers take highest priority)
Also, it should be possible for this value to be overridden by developers.
| return SyntaxInfo.of(origin, type, patterns); | ||
| } | ||
|
|
||
| public static final class Expression<E extends ch.njol.skript.lang.Expression<R>, R> { |
There was a problem hiding this comment.
IMO these builders should extend the generic builder class and just add on their additional methods (ExpressionType, EntryValidator)
There was a problem hiding this comment.
I considered this but that would be entering a generic hell. Might change this later.
There was a problem hiding this comment.
Could be a little rough, but worth pursuing IMO
src/main/java/org/skriptlang/skript/registration/SkriptRegistryImpl.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Patrick Miller <[email protected]> Co-authored-by: LimeGlass <[email protected]>
Moderocky
force-pushed
the
master
branch
2 times, most recently
from
bd134d0
to
3f08853
Compare
|
Closing in favor of #6246 |
Description
This is the first PR in an effort to split Skript from api, the parser implementation and Bukkit implementation. The final goal is to have
org.skriptlang.skriptas main package for all api in relation to Skript, with a default implementation for the parser. The Bukkit implementation would be elsewhere.Currently the API is marked as experimental so changes can still be applied, but merging this does allow addons to start using the new API. This PR is focused on the migrating the registration process to the new API, without breaking changes.