-
-
Notifications
You must be signed in to change notification settings - Fork 407
Fixes warning for some events cannot be cancelled #8102
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
base: dev/patch
Are you sure you want to change the base?
Fixes warning for some events cannot be cancelled #8102
Conversation
Co-authored-by: Patrick Miller <apickledwalrus@gmail.com>
Co-authored-by: Patrick Miller <apickledwalrus@gmail.com>
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. |
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 |
src/main/java/org/skriptlang/skript/lang/script/ScriptWarning.java
Outdated
Show resolved
Hide resolved
…java Co-authored-by: sovdee <10354869+sovdeeth@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
And then it doesn't have to be a JUnit test anymore |
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:
Skript/src/main/java/ch/njol/skript/effects/EffCancelEvent.java
Line 76 in 1132757
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:
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