Rewrite Custom Syntax#165
Conversation
# Conflicts: # build.gradle # src/main/java/com/btk5h/skriptmirror/ParseOrderWorkarounds.java # src/main/java/com/btk5h/skriptmirror/SkriptMirror.java # src/main/java/com/btk5h/skriptmirror/skript/custom/ExprExpression.java # src/main/java/com/btk5h/skriptmirror/skript/custom/ExprParseTags.java # src/main/java/com/btk5h/skriptmirror/skript/custom/ExprRawExpression.java # src/main/java/com/btk5h/skriptmirror/skript/reflect/ExprJavaCall.java # src/main/java/org/skriptlang/reflect/syntax/custom/event/EventValuesEntryData.java # src/main/java/org/skriptlang/reflect/syntax/custom/expression/ChangerEntryData.java # src/main/java/org/skriptlang/reflect/syntax/event/elements/CustomEvent.java # src/main/java/org/skriptlang/reflect/syntax/event/elements/EffCallEvent.java # src/main/java/org/skriptlang/reflect/syntax/event/elements/ExprCustomEvent.java # src/main/java/org/skriptlang/reflect/syntax/event/elements/ExprCustomEventValue.java # src/main/java/org/skriptlang/reflect/syntax/event/elements/ExprEventData.java # src/main/java/org/skriptlang/reflect/syntax/event/elements/ExprReplacedEventValue.java # src/main/java/org/skriptlang/reflect/syntax/event/elements/StructCustomEvent.java # src/main/java/org/skriptlang/reflect/syntax/expression/elements/ExprChangeValue.java # src/main/java/org/skriptlang/reflect/syntax/expression/elements/StructCustomExpression.java
…r than the ParserInstance
sovdeeth
left a comment
There was a problem hiding this comment.
I haven't looked through super closely but this seems fantastic from skimming it
No glaring issues (except the lack of docs annotations!)
origins and priorities should probably be handled automatically for the most part (may want to use an origin-applying registry)
The logic for the custom events looks great, good usage of ERS
# Conflicts: # build.gradle
…ed custom event classes
sovdeeth
left a comment
There was a problem hiding this comment.
Still have 12 files to go but it looks really solid tud
APickledWalrus
left a comment
There was a problem hiding this comment.
Truly excellent work!
I'm going to ask that you add Documentation annotations for all of these elements. It would be nice to include skript-reflect on the new docs website 🙂
Side question: Shouldn't the new package be something like org.skriptlang.skriptreflect? (akin to org.skriptlang.skriptworldguard). I feel like the organization is a bit odd in general too. I think it might make sense to have a single subpackage like elements with further subpackages java, syntax, etc.
There was a problem hiding this comment.
This whole class is really questionable and I would move to get rid of it completely if possible.
There was a problem hiding this comment.
sure this was before the priority stuff were a thing
|
|
||
| CustomEventManager.defineCustomEvent(identifier); | ||
|
|
||
| return super.preLoad() && super.load(); |
There was a problem hiding this comment.
why is this? it doesn't seem to have an impact since these methods don't do much in the case of custom events, but it reads kind of badly
There was a problem hiding this comment.
they're calling the super methods, which handle registering the custom element and parsing the parse entry. it needs to happen at init for events since events are structures and calling these methods after init would be too late
Problem
The current custom syntax implementation is built on very old Skript API, when modifying syntax registries at runtime wasn't natively supported. This led to code relying on hacky methods and workarounds to achieve this type of behavior, as a byproduct, the codebase for this system was extremely hard to follow, fragmented and difficult to maintain.
Solution
Rewrite the custom syntax system from the ground up to use the registration API, significantly improving its readability, maintainability and consistency.
Testing Completed
Manual testing
Supporting Information
This rewrite should not introduce any user-facing breaking changes
Review Notice
If you are reviewing this pull request, I suggest starting from the
org.skriptlang.reflect.syntax.custom.sharedpackage, as everything is built on top of the base classes found there.Completes: none
Related: none
AI assistance: none