Skip to content

Replace the manual isCurrentEvent checks in favor of ERS#8529

Open
NotroDev wants to merge 13 commits intoSkriptLang:dev/patchfrom
NotroDev:patch/event-restricted-syntax-refactor
Open

Replace the manual isCurrentEvent checks in favor of ERS#8529
NotroDev wants to merge 13 commits intoSkriptLang:dev/patchfrom
NotroDev:patch/event-restricted-syntax-refactor

Conversation

@NotroDev
Copy link
Copy Markdown

@NotroDev NotroDev commented Apr 7, 2026

Problem

The PR introducing the EventRestrictedSyntax interface didn't replace the usages in the legacy classes, so they still used the manual isCurrentEvent checks in their init() methods. While it works pretty much the same, the ERS approach is cleaner and can be used to extract the info about supported events.

Solution

I just went over all the classes where the old approach was still used and replaced it with the ERS implementation.

Testing Completed

I didn't do any manual testing (as that's a technical change) but it compiles and passes the quickTest.

Supporting Information

The ERS interface is pretty straightforward so I could replace only the most basic checks.
For example, the ExprProtocolVersion class checks for the existence of the paper event first:

@Override
public boolean init(Expression<?>[] exprs, int matchedPattern, Kleenean isDelayed, ParseResult parseResult) {
	if (!PAPER_EVENT_EXISTS) {
		Skript.error("The protocol version expression requires Paper 1.12.2 or newer");
		return false;
	} else if (!getParser().isCurrentEvent(PaperServerListPingEvent.class)) {
		Skript.error("The protocol version expression can't be used outside of a server list ping event");
		return false;
	}
	return true;
}

And I don't think I can recreate this behavior properly in ERS without compromising the proper error messages (and I didn't want to expand the ERS logic without discussion)

The only exception is EffHidePlayerFromServerList where I used this approach:

@Override
public Class<? extends Event>[] supportedEvents() {
	if (!PAPER_EVENT_EXISTS) {
		return CollectionUtils.array(ServerListPingEvent.class);
	}
	else {
		return CollectionUtils.array(ServerListPingEvent.class, PaperServerListPingEvent.class);
	}
}

except in that case I believe the PaperServerListPingEvent is redundant anyway (and because it inherits from the ServerListPingEvent, the isCurrentEvent which uses isAssignableFrom under the hood, will pass it anyway)

There are also much more complicated examples in the Skript codebase where the supported events change based on the pattern or other circumstances, so the ERS couldn't be implemented.

PS. I can't guarantee that I've found all the usages that could be replaced :P


Completes: none
Related: none
AI assistance: none (i did it fully by hand because I thought there would be far fewer legacy usages...)

@NotroDev NotroDev requested a review from a team as a code owner April 7, 2026 08:55
@NotroDev NotroDev requested review from TheMug06 and erenkarakal and removed request for a team April 7, 2026 08:55
@skriptlang-automation skriptlang-automation bot added the needs reviews A PR that needs additional reviews label Apr 7, 2026
Copy link
Copy Markdown
Member

@Efnilite Efnilite left a comment

Choose a reason for hiding this comment

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

nice work, one tiny thing

Co-authored-by: Efnilite <35348263+Efnilite@users.noreply.github.com>
@skriptlang-automation skriptlang-automation bot removed the needs reviews A PR that needs additional reviews label Apr 7, 2026
@UnderscoreTud
Copy link
Copy Markdown
Member

And I don't think I can recreate this behavior properly in ERS without compromising the proper error messages (and I didn't want to expand the ERS logic without discussion)

You could add support for this while you're at it, It's pretty simple. Just start a retaining log handler before supportedEvents is called, check if it contains any errors after, and print them if you find any. Then Skript.error should just work with it

@APickledWalrus
Copy link
Copy Markdown
Member

And I don't think I can recreate this behavior properly in ERS without compromising the proper error messages (and I didn't want to expand the ERS logic without discussion)

You could add support for this while you're at it, It's pretty simple. Just start a retaining log handler before supportedEvents is called, check if it contains any errors after, and print them if you find any. Then Skript.error should just work with it

It is worth noting that ERS checks occur before init, so any init-dependent behavior is still unable to be updated. Custom error messages may already work though.

@skriptlang-automation skriptlang-automation bot added the patch-ready A PR/issue that has been approved and is ready to be merged/closed for the next patch version. label Apr 7, 2026
@NotroDev
Copy link
Copy Markdown
Author

NotroDev commented Apr 9, 2026

You could add support for this while you're at it, It's pretty simple. Just start a retaining log handler before supportedEvents is called, check if it contains any errors after, and print them if you find any. Then Skript.error should just work with it

I'll try to look into it when I have some free time, so I think this PR can stay open for a while

@Efnilite Efnilite added the housekeeping Housekeeping-related issue or task. label Apr 9, 2026
@NotroDev
Copy link
Copy Markdown
Author

Hey, I've gotten some work done.

First, after @Absolutionism's comment, I realized that most of the "paper event exists" checks were redundant. I cleaned them up and added ERS where possible.

The changes in ExprHoverList were bigger because I cut all the fallback logic when HAS_NEW_LISTED_PLAYER_INFO fails. This field was introduced in 1.20.6, which is 23 months old.
If that's not how the Skript support lifetime works, please let me know 😅

next I'll try to expand the ERS usage as discussed above

Copy link
Copy Markdown
Contributor

@Absolutionism Absolutionism left a comment

Choose a reason for hiding this comment

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

Just some coding convention touch ups

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

housekeeping Housekeeping-related issue or task. patch-ready A PR/issue that has been approved and is ready to be merged/closed for the next patch version.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants