-
-
Notifications
You must be signed in to change notification settings - Fork 377
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
Dynamic function calling. #6713
Dynamic function calling. #6713
Conversation
Co-authored-by: Patrick Miller <[email protected]>
What about #6254? |
also see #6254 (comment). i have to be honest - i'm not really a fan of reflective capabilities in vanilla skript |
I, too, am not a big fan. |
Sorry if im blanking right now, but why would you even need this in the first place? you could just... run the function? i dont see any point for this atleast in my eyes |
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.
Are you able to update this since the main PR was merged? Feel free to re-request after
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 love the concept. Mainly some formatting suggestions/minor syntax corrections.
A few general ideas:
- I think it would be good if we could find a way to avoid the specific checks within syntax for DynamicFunctionReference. I also understand if that cannot be reasonably moved though. Can also be done in a future PR
src/main/java/ch/njol/skript/lang/function/DynamicFunctionReference.java
Outdated
Show resolved
Hide resolved
src/main/java/ch/njol/skript/lang/function/DynamicFunctionReference.java
Outdated
Show resolved
Hide resolved
src/main/java/ch/njol/skript/lang/function/DynamicFunctionReference.java
Outdated
Show resolved
Hide resolved
src/main/java/ch/njol/skript/lang/function/DynamicFunctionReference.java
Outdated
Show resolved
Hide resolved
src/main/java/ch/njol/skript/lang/function/DynamicFunctionReference.java
Show resolved
Hide resolved
Co-authored-by: Patrick Miller <[email protected]>
import java.io.File; | ||
import java.util.ArrayList; | ||
import java.util.List; | ||
|
||
@Name("Name / Display Name / Tab List 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.
Just a note that this is going to need changing if Any X gets merged first.
.examples("run {_function} with arguments 1 and true", | ||
"set {_result} to the result of {_function}") | ||
.since("INSERT VERSION") | ||
.parser(new Parser<DynamicFunctionReference<?>>() { |
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.
can this be a named inner 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.
I'm not sure what you mean by this.
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.
Like private static class DynamicFunctionReferenceParser extends Parser...
. I did this in the JavaClasses rework to make the registration section easier to navigate, however its not that big of a deal as most parsers are just an anonymous 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.
I see. I'm not sure this would add a huge amount of value, since it would just be moved to another place, not made any smaller.
|
||
@Override | ||
public String toString(@Nullable Event event, final boolean debug) { | ||
String text = "the result" + (isPlural ? "s" : "") + " of " + getExpr().toString(event, debug); |
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.
SyntaxStringBuilder
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.
In its current state I don't think SyntaxStringBuilder has any use apart from getting to skip the toString
bits, since you can't chain append calls.
It's also not useful with propertyexpressions right now because of the annotation mismatch.
Right now, it's just significantly more mess than doing it normally.
Once these shortcomings have been improved I might consider using it.
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.
Most of those issues have been fixed, so you should be fine to use it now. Rather than chaining (although now supported), you can just pass multiple arguments:
builder.append("with arguments", arguments);
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 few final things
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 good
|
||
@Override | ||
public String toString(@Nullable Event event, final boolean debug) { | ||
String text = "the result" + (isPlural ? "s" : "") + " of " + getExpr().toString(event, debug); |
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.
Most of those issues have been fixed, so you should be fine to use it now. Rather than chaining (although now supported), you can just pass multiple arguments:
builder.append("with arguments", arguments);
1221314
into
SkriptLang:feature/script-reflection
* Create Script type. * Support script string/name conversion. * Script expression. * Add script lang entry. * Tests for script expression & names. * Support all scripts expression. * Script effects & tests. * Create dummy Script handle for disabled scripts. * Script reflection feature flag. * Restrict literal parsing to commands & parse. * Test feature flag for resolving name. * Split ExprScripts by feature to support disabled scripts. * Fix ExprName tests for new & old behaviour. * Add tests for disabled script handles. * Apply suggestions from code review Co-authored-by: Patrick Miller <[email protected]> * Improve script loading/unloading safety. * Add feature check for script hotswapping. * Use expression stream. * Conformity for file names and proper loading safety. * Document validity & add condition support. * Add script is loaded condition + tests. * Dynamic function calling + tests. * Add language entry for types. * Single-encounter input bootstrapping. * Apply suggestions from code review Co-authored-by: sovdee <[email protected]> * Fix inspection. * Update src/main/java/ch/njol/skript/expressions/ExprFunction.java Co-authored-by: sovdee <[email protected]> * Update src/main/java/ch/njol/skript/expressions/ExprResult.java Co-authored-by: sovdee <[email protected]> * Changes from review. * Fix merge problems. * Remove script command method usage. * Fix branch muck. * Fix more branch muck. * Fix up ExprName. * Add docs. * Add docs. * Add docs. * Fix bits. * Apply suggestions from code review Co-authored-by: Patrick Miller <[email protected]> * Fix some bits. * Function parsing by name. * Fix up some bits for Walrus. * Fix merge error. --------- Co-authored-by: Patrick Miller <[email protected]> Co-authored-by: sovdee <[email protected]>
* Create Script type. * Support script string/name conversion. * Script expression. * Add script lang entry. * Tests for script expression & names. * Support all scripts expression. * Script effects & tests. * Create dummy Script handle for disabled scripts. * Script reflection feature flag. * Restrict literal parsing to commands & parse. * Test feature flag for resolving name. * Split ExprScripts by feature to support disabled scripts. * Fix ExprName tests for new & old behaviour. * Add tests for disabled script handles. * Apply suggestions from code review Co-authored-by: Patrick Miller <[email protected]> * Improve script loading/unloading safety. * Add feature check for script hotswapping. * Use expression stream. * Conformity for file names and proper loading safety. * Document validity & add condition support. * Add script is loaded condition + tests. * Dynamic function calling + tests. * Add language entry for types. * Single-encounter input bootstrapping. * Apply suggestions from code review Co-authored-by: sovdee <[email protected]> * Fix inspection. * Update src/main/java/ch/njol/skript/expressions/ExprFunction.java Co-authored-by: sovdee <[email protected]> * Update src/main/java/ch/njol/skript/expressions/ExprResult.java Co-authored-by: sovdee <[email protected]> * Changes from review. * Fix merge problems. * Remove script command method usage. * Fix branch muck. * Fix more branch muck. * Fix up ExprName. * Add docs. * Add docs. * Add docs. * Fix bits. * Apply suggestions from code review Co-authored-by: Patrick Miller <[email protected]> * Fix some bits. * Function parsing by name. * Fix up some bits for Walrus. * Fix merge error. --------- Co-authored-by: Patrick Miller <[email protected]> Co-authored-by: sovdee <[email protected]>
* Create Script type. * Support script string/name conversion. * Script expression. * Add script lang entry. * Tests for script expression & names. * Support all scripts expression. * Script effects & tests. * Create dummy Script handle for disabled scripts. * Script reflection feature flag. * Restrict literal parsing to commands & parse. * Test feature flag for resolving name. * Split ExprScripts by feature to support disabled scripts. * Fix ExprName tests for new & old behaviour. * Add tests for disabled script handles. * Apply suggestions from code review Co-authored-by: Patrick Miller <[email protected]> * Improve script loading/unloading safety. * Add feature check for script hotswapping. * Use expression stream. * Conformity for file names and proper loading safety. * Document validity & add condition support. * Add script is loaded condition + tests. * Dynamic function calling + tests. * Add language entry for types. * Single-encounter input bootstrapping. * Apply suggestions from code review Co-authored-by: sovdee <[email protected]> * Fix inspection. * Update src/main/java/ch/njol/skript/expressions/ExprFunction.java Co-authored-by: sovdee <[email protected]> * Update src/main/java/ch/njol/skript/expressions/ExprResult.java Co-authored-by: sovdee <[email protected]> * Changes from review. * Fix merge problems. * Remove script command method usage. * Fix branch muck. * Fix more branch muck. * Fix up ExprName. * Add docs. * Add docs. * Add docs. * Fix bits. * Apply suggestions from code review Co-authored-by: Patrick Miller <[email protected]> * Fix some bits. * Function parsing by name. * Fix up some bits for Walrus. * Fix merge error. --------- Co-authored-by: Patrick Miller <[email protected]> Co-authored-by: sovdee <[email protected]>
Additions
New types
Obtain function references by name (& source)
Run function references & get results
Little examples
Infrastructure
Road-map of future plans
I'm setting down a bit of API here for future Skript features/addons to use,
especially ones that want to run tasks.
I have a few ideas about what I would like to do with this, but nothing finalised yet.
Target Minecraft Versions: any
Requirements: none
Related Issues: fixes #1265, fixes #5838, requires #6702