Skip to content

Adds a preinit() method to SyntaxElement #7778

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

Merged
merged 5 commits into from
Jun 3, 2025

Conversation

sovdeeth
Copy link
Member

@sovdeeth sovdeeth commented Apr 5, 2025

Description

I first ran into this issue with runtime errors, where I wanted code to run in init for all expressions. This was not possible for any class but SimplePropertyExpression, as it was the only one that used a separate init method for children and therefore could run code for all children.

This adds a preinit() method to SyntaxElement that is called just before init() and is intended to allow any syntax element the chance to do checks or work that should be consistent across children, without needing to change the signature of init() for the children or force all implementations to call super.init().

I also took the opportunity to separate ERS and ExprimentalSyntax's logic in SkriptParser into helper methods for clarity.

This PR includes changes to runtime errors to showcase the utility of preinit. It's not ideal, as I'd like to get the node in Expression, but state isn't allowed in interfaces so SimpleExpression it is.

I welcome feedback about whether this is necessary or if there are better ways to implement this kind of feature.


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

@sovdeeth sovdeeth added the feature Pull request adding a new feature. label Apr 5, 2025
@sovdeeth sovdeeth requested review from a team as code owners April 5, 2025 03:10
@sovdeeth sovdeeth requested review from Romitou, erenkarakal and UnderscoreTud and removed request for a team April 5, 2025 03:10
@APickledWalrus APickledWalrus self-requested a review April 5, 2025 03:36
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.

seems useful. should be fine, i just have a few thoughts

@sovdeeth sovdeeth moved this to In Review in 2.12 Release May 14, 2025
@skriptlang-automation skriptlang-automation bot added the feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. label May 15, 2025
@sovdeeth sovdeeth merged commit 77173b2 into SkriptLang:dev/feature Jun 3, 2025
5 checks passed
@github-project-automation github-project-automation bot moved this from Awaiting Merge to Done in 2.12 Release Jun 3, 2025
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. feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants