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

Dynamic Functions #6254

Closed
wants to merge 1 commit into from
Closed

Conversation

UnderscoreTud
Copy link
Member

Description

This PR adds the long-awaited feature to Skript, dynamic functions! Functions are now objects that can be dealt with inside scripts with the new function type. To get a function object, you use the ExprFunction expression.
Example:

set {_func} to function "sum"

You can even get local functions from other scripts, like so:

set {_func} to function "foo" in script "hello"

Using these objects you can execute or get the result of a function's execution dynamically, like so:

set {_func} to function "product"
set {_result} to execution result of {_func} with arguments 3, 4 and 5
broadcast {_result} # 60

Target Minecraft Versions: any
Requirements: none
Related Issues: #1265

@UnderscoreTud UnderscoreTud added feature Pull request adding a new feature. 2.8 Targeting a 2.8.X version release functions Related to functions labels Dec 25, 2023
@Pikachu920
Copy link
Member

I like the idea of being able to call functions dynamically, but I'm hesitant to add functions as first-class objects.

@UnderscoreTud
Copy link
Member Author

I like the idea of being able to call functions dynamically, but I'm hesitant to add functions as first-class objects.

What do you suggest? I did it so the patterns are not too long with the ability to specify which script the function is declared in, and also it doesn't look pretty to shove everything into one pattern like so: (call|execute|run) [the] [:global] function[s] [named] %strings% [(in|from) [script] [file] %-string%] [(using|with) [[the] (argument|parameter)[s]] %-objects%]. It's just a mess, implementation wise as well

Copy link
Member

@AyhamAl-Ali AyhamAl-Ali left a comment

Choose a reason for hiding this comment

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

Well done Tud ⚡ I like this feature


@Name("Execute Function")
@Description("Executes a function with the given parameters.")
@Examples("run function \"rotate_%{_direction}%\" with {_structure-name}")
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer having a more in depth example for this specific advanced feature, like add 2 functions with the possible names and params

Copy link
Member Author

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

Copy link
Member

Choose a reason for hiding this comment

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

Here is an example which you can commit if you like :)

Suggested change
@Examples("run function \"rotate_%{_direction}%\" with {_structure-name}")
@Examples({
"function rotate_north(structure: text):",
"\t# code",
"",
"function rotate_east(structure: text):",
"\t# code",
"",
"run function \"rotate_%{_direction}%\" with {_structure-name}"
})

}

private static String formatScript(String script) {
if (!script.endsWith(".sk")) script += ".sk";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (!script.endsWith(".sk")) script += ".sk";
if (!script.endsWith(".sk"))
script += ".sk";


Object[][] params = new Object[singleListParam ? 1 : parameters.length][];
if (singleListParam && parameters.length > 1) { // All parameters to one list
List<Object> l = new ArrayList<>();
Copy link
Member

Choose a reason for hiding this comment

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

Let's use better naming here please.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just moved the validation code from FunctionReference and moved it here. I guess I could clean it up as well

Copy link
Member

Choose a reason for hiding this comment

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

That would be nice :)

@Pikachu920
Copy link
Member

I like the idea of being able to call functions dynamically, but I'm hesitant to add functions as first-class objects.

What do you suggest? I did it so the patterns are not too long with the ability to specify which script the function is declared in, and also it doesn't look pretty to shove everything into one pattern like so: (call|execute|run) [the] [:global] function[s] [named] %strings% [(in|from) [script] [file] %-string%] [(using|with) [[the] (argument|parameter)[s]] %-objects%]. It's just a mess, implementation wise as well

that is the route i would take, but i would make it a few patterns. what would make the implementation messy?

@sovdeeth sovdeeth removed the 2.8 Targeting a 2.8.X version release label Dec 30, 2023
@Pikachu920 Pikachu920 added the up for debate When the decision is yet to be debated on the issue in question label Dec 30, 2023
@ElijahJunaid
Copy link

i think adding this would be useful, less lines to parse and probably an overall quicker way to run stuff.
addons already have it and for those who do not wish to add addons, this would benefit them.

@TheLimeGlass TheLimeGlass self-requested a review January 19, 2024 10:14
@sovdeeth
Copy link
Member

Closing due to inactivity and in favor of #6713

@sovdeeth sovdeeth closed this Jul 16, 2024
@APickledWalrus APickledWalrus deleted the feature/dynamic-functions branch October 13, 2024 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Pull request adding a new feature. functions Related to functions 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.

5 participants