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

Script type & expression. #6702

Open
wants to merge 28 commits into
base: feature/script-reflection
Choose a base branch
from

Conversation

Moderocky
Copy link
Member

@Moderocky Moderocky commented May 17, 2024

Description

The first step towards supporting proper reflection & introspection: a type and object handles for scripts.

This adds:

  1. A script type.
    • Internal dummy Script objects for disabled/unlinked scripts.
  2. A feature flag [script] reflection for most linked features & breaking changes.
  3. Internal Validated interface to be used by scripts and other resources that could become obsolete. This is purely a testing API right now.
  4. An expression for [the] [current] script, [the] script[s] [named] %strings%.
  5. Handler for the name property of scripts.
    • If the feature flag is disabled, returns exactly what name of script did, e.g. scripts/folder/MyScript.sk -> folder/MyScript.
    • Otherwise, returns the name of the file without its path/extension, e.g. scripts/folder/MyScript.sk -> MyScript.
    • The path-relative version is available by just stringifying a script object now.
  6. The enable/disable/reload script effects now take in a script object in preference to a string (which equates to its original syntax).
    • This uses the internal Script dummy object for disabled scripts.
  7. The legacy enable/disable script pattern has been separated into a more reasonable arrangement.
    • Technically, this is a breaking change, since you can no longer do enable skript "hello.sk"
    • I found the mixing of skript and script to be unhelpful and confusing, people might mistake reload skript ... to mean reloading Skript, when it was actually for files.
    • Now, it explicitly takes a skript file ..., or script... to make that clearer.
  8. Tests for all of the above.

Breaking Changes

The enable|disable skript %string% pattern variation was removed. You can still use skript file or script.

Resource safety has been significantly improved: there were basically zero checks on what you tried to reload/disable previously. If it wasn't a valid file it would throw huge exceptions, and if it was it would just get processed without any kind of safety checks.

  • Enabling a non-disabled-script-file would just remove the first character of its name and try to parse it. Obviously, this has been corrected.
  • It was (previously) fine to enable or disable files outside the working directory. This could be open to significant exploit, so now file paths are checked vs. the canonical scripts/ directory (unless you're running in Test mode!)
    • I'm sure some people were using this meaningfully to load external scripts, but it really just shouldn't be permissible. Fortunately, the next change should help with that.
  • Attempting to load a loaded script, unload a non-loaded script, etc. would previously throw a massive error. Now it will silently skip them.

With the experiment enabled, unloading is now considered different from disabling in syntax: disabling a script will unload it and prepend - to its file name. Unloading it will just unload it from memory (but not disable it, so it would load normally upon next restart).

Where Next?

This could be the start of some reflection.
I'd like to see a bit of introspection in the future (maybe looking at what structures are in a script, some features from Config, etc.)

There was some discussion about some other reflective stuff (like dynamic function calls) which would probably tie into this quite nicely. I've opened a branch for anybody who wants it.

This could move in the direction of runtime-parsed scripts, anonymous scripts, etc.

I'm also wondering about abstracting some kind of loadable interface (or maybe just use Config) so that we can permit the [re]loading of other Skript-related things that can be loaded (aliases, config, etc.)
I don't have any plans for the loading of non-script-provided files (e.g. a user-provided config .sk file) at this point, but if I abstracted this back a layer then it would be really easy (for an addon) to provide that in the future if somebody wants to.


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

@Moderocky Moderocky added enhancement Feature request, an issue about something that could be improved, or a PR improving something. breaking changes Pull or feature requests that contain breaking changes (API, syntax, etc.) labels May 17, 2024
@APickledWalrus APickledWalrus changed the base branch from master to dev/feature May 17, 2024 17:00
@Moderocky Moderocky changed the base branch from dev/feature to feature/script-reflection May 17, 2024 17:04
Copy link
Member

@APickledWalrus APickledWalrus left a comment

Choose a reason for hiding this comment

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

quick first impressions

src/main/java/ch/njol/skript/expressions/ExprScript.java Outdated Show resolved Hide resolved
src/test/skript/tests/misc/-disabled.sk Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/expressions/ExprScript.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffScriptFile.java Outdated Show resolved Hide resolved
Copy link
Member

@APickledWalrus APickledWalrus left a comment

Choose a reason for hiding this comment

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

Looking good. One major thing lots of switches can be replaced with switch expressions now that they are available. Additionally, do you think it is worth having an expression like path of %scripts% or should we just keep it to the toString? I'm not sure how far we want to go in terms of file-type syntax.

@Moderocky
Copy link
Member Author

Looking good. One major thing lots of switches can be replaced with switch expressions now that they are available. Additionally, do you think it is worth having an expression like path of %scripts% or should we just keep it to the toString? I'm not sure how far we want to go in terms of file-type syntax.

I think I kept the toString as the path because we have name of X for the name.

@sovdeeth
Copy link
Member

damnit i can never merge properly
sorry kenzie

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking changes Pull or feature requests that contain breaking changes (API, syntax, etc.) enhancement Feature request, an issue about something that could be improved, or a PR improving something.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants