Example Script API #8191
Example Script API #8191
Conversation
cheeezburga
requested review from
APickledWalrus and
Pesekjak
and removed request for
a team
skriptlang-automation
bot
added
the
needs reviews
|
Haven't looked over this much but regarding your comment about it always readding the examples can we not just add a config toggle? And does this load example skripts on server start or is it unloaded until reloaded? |
| /** | ||
| * Represents an example script bundled with Skript or an addon. | ||
| */ | ||
| public record ExampleScript(String name, String content) { |
There was a problem hiding this comment.
maybe this could be an interface where all the example scripts are registered? this can then be extended by addons and registered with SkriptAddon
There was a problem hiding this comment.
I've added in an ExampleScriptProvider interface, is that sort of what you were thinking with this?
skriptlang-automation
bot
removed
the
needs reviews
- Also fixes some other Efy suggestions, like making exampleManager package private, etc.
I think there was a bug or two in the initial implementation, but all sorted now. Done some testing and it all works as intended, as far as I can see. |
| private static ExampleScript load(String name) { | ||
| String path = "scripts/-examples/" + name; | ||
| try (InputStream in = CoreExampleScripts.class.getClassLoader().getResourceAsStream(path)) { | ||
| if (in == null) | ||
| throw new IllegalStateException("Missing example script " + path); | ||
| String content = new String(in.readAllBytes(), StandardCharsets.UTF_8); | ||
| return new ExampleScript(name, content); | ||
| } catch (IOException e) { | ||
| throw new RuntimeException("Failed to load example script " + path, e); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
some duplicate code here, can we de duplicate it?
| /** | ||
| * Persists the list of installed example scripts. Hides the metadata file on Windows systems. | ||
| */ | ||
| private void flushInstalled() { |
There was a problem hiding this comment.
flush implies deletion/removal to me. Perhaps a better name would be writeTrackingFile or similar
| boolean dirty = false; | ||
| File baseDir = new File(scriptsDir, "-examples/" + plugin); | ||
| for (ExampleScript script : scripts) { | ||
| String key = plugin + "/" + script.name(); |
There was a problem hiding this comment.
are there any concerns about operating systems and different path separation characters? I know we use File.separator elsewhere.
4 participants
Problem
The current method of providing the example scripts isn't ideal, and doesn't allow addons to extend this useful function.
This PR gives addons a way to add their own example scripts, allowing for another method of providing documentation to new and experienced Skript players alike.
Solution
This PR introduces a new way to create and track the example scripts. It now uses a hidden
.examples-loadedfile in the Skript folder to identify which examples need to be created on plugin enable, and which have already been handled. This is a better alternative, as before Skript used to just go off of whether the scripts folder existed, which isn't a sure way to get this info.The example scripts are stored as
ExampleScriptrecords, and loaded from the resources where they're currently stored.It also provides
SkriptAddon#registerExampleScriptsso that addons are able to provide example scripts. With this change, the example scripts have been separated into different folders based on the addon name.Testing Completed
Manually tested going from old versions of Skript to this one, and from this version of Skript to itself.
Haven't included tests for this as I'm unsure how to validate that, once Skript has loaded them once, they don't get loaded again. My own testing confirms that the behaviour works as intended, but unsure how to prove it via code.
Supporting Information
With this PR, given the updated way that example scripts are created and tracked, all example scripts will be created again if they were deleted. I think this is pretty minor, and don't think it's possible to avoid.
Not massively attached to this implementation, just thought it might be a nice addition and put together a solution quickly, so thoughts and feedback are appreciated.
Completes: none
Related: none