Skript icon indicating copy to clipboard operation
Skript copied to clipboard

Fixes warning for some events cannot be cancelled

Open TheLimeGlass opened this issue 6 months ago • 6 comments

Problem

The current event system of Skript will error if any single event cannot be cancelled in it's registered listening events. For example, if an event was registered that listened to a cancellable event and a non cancellable event. Skript would either pass it or fail it depending on the registration order. Not ideal.

See existing TODO note placed by Njol: https://github.com/SkriptLang/Skript/blob/11327579a9af912221ba6e0864d937935ead8799/src/main/java/ch/njol/skript/effects/EffCancelEvent.java#L76

Solution

This pull request fixes that issue, and produces a suppressible warning if any of the events cannot be cancellable. If all events cannot be cancelled, then a full error is produced like current behavior.

JUnit beneficial changes needed to accommodate:

  • Allowed all test registration to be present in the DEV ENVIRONMENT for self testing.
  • DEV ENVIRONMENT now uses the JUnit test jar to allow for usage of JUnit and custom test syntaxes.
  • JUnit objectives can now be completed programmatically with static method EffObjectives#complete and not just through a Skript ran effect.

Made usage of the built in JUnit custom registration system I made awhile ago but never used. We now have a valid example of making custom events for tests.

Testing Completed


Completes: none Related: none

TheLimeGlass avatar Aug 01 '25 10:08 TheLimeGlass

I've asked you repeatedly to follow the correct PR template. You appear to be deliberately choosing to not use it in favor of the outdated one we've replaced. It's not the biggest issue but it shows an unacceptable disregard for our contribution standards and the team. Please update the PR description to use the current template.

sovdeeth avatar Aug 01 '25 17:08 sovdeeth

I've asked you repeatedly to follow the correct PR template. You appear to be deliberately choosing to not use it in favor of the outdated one we've replaced. It's not the biggest issue but it shows an unacceptable disregard for our contribution standards and the team. Please update the PR description to use the current template.

Updated

TheLimeGlass avatar Aug 01 '25 17:08 TheLimeGlass

Updated

Thank you. The fields at the end are incorrect (should show Completed: and Related:). You are also missing a section for any testing you completed. Please use the correct template from the get-go next time :)

sovdeeth avatar Aug 01 '25 17:08 sovdeeth

Updated

Thank you. The fields at the end are incorrect (should show Completed: and Related:). You are also missing a section for any testing you completed. Please use the correct template from the get-go next time :)

Updated

TheLimeGlass avatar Aug 01 '25 17:08 TheLimeGlass

Rather than having to execute the events, I'd recommend using StructParse which should simplify the test implementation: https://github.com/SkriptLang/Skript/blob/master/src/main/java/ch/njol/skript/test/runner/StructParse.java https://github.com/SkriptLang/Skript/blob/master/src/test/skript/tests/syntaxes/structures/StructParse.sk (note that this tests a nested example)

It does use the parse sections. Not the structure, because the point is to test cancellation events, not a wrapper using another parsed structure. It should remain using the same implementation a Skripter would interact with. Avoid the probability of the other structure causing logging desync or not true parsing.

Example being how async effects and expressions within sections from skript reflect don't properly print logger errors currently due to the thread being async or multi threading parsing option.

TheLimeGlass avatar Aug 11 '25 03:08 TheLimeGlass

Rather than having to execute the events, I'd recommend using StructParse which should simplify the test implementation: https://github.com/SkriptLang/Skript/blob/master/src/main/java/ch/njol/skript/test/runner/StructParse.java https://github.com/SkriptLang/Skript/blob/master/src/test/skript/tests/syntaxes/structures/StructParse.sk (note that this tests a nested example)

It does use the parse sections. Not the structure, because the point is to test cancellation events, not a wrapper using another parsed structure. It should remain using the same implementation a Skripter would interact with. Avoid the probability of the other structure causing logging desync or not true parsing.

Example being how async effects and expressions within sections from skript reflect don't properly print logger errors currently due to the thread being async or multi threading parsing option.

The execution of the events doesn't need to be tested/occur though. Something like this would would test the same thing without having to handle executing the events:

parse:
	results: {cancellable::1::*}
	code:
		on cancel events test 1:
			cancel event

test "multiple cancellable events":
	assert {cancellable::1::*} is "An on cancel events test 1 event can be triggered for multiple events, some of which cannot be cancelled." with "Failed to parse or grab cancel events 1 warning"

And then it doesn't have to be a JUnit test anymore

APickledWalrus avatar Aug 24 '25 01:08 APickledWalrus