Skip to content
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

Configuration for custom events #5724

Open
wants to merge 10 commits into
base: dev/feature
Choose a base branch
from

Conversation

TheLimeGlass
Copy link
Collaborator

Description

Adds a configuration for registering simple events, ported from Skellett.


Target Minecraft Versions: any
Requirements: none
Related Issues: #121 (non-closing)

(cherry picked from commit 07e1700)
@TheLimeGlass TheLimeGlass added the feature Pull request adding a new feature. label May 31, 2023
@TheLimeGlass TheLimeGlass mentioned this pull request May 31, 2023
11 tasks
Copy link
Member

@sovdeeth sovdeeth left a comment

Choose a reason for hiding this comment

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

Cool pr!
I am a little worried about users adding async events and then having issues using syntax inside those events (like the chat event in 2.7 betas), but I suppose it's on their heads if they add an async event. Perhaps a warning about that in the file would be good.

edit: Apparently i forgot to save this comment:
You can leave the name blank, which doesn't seem to cause issues but should not be allowed imo:
image

src/main/resources/custom-events.sk Outdated Show resolved Hide resolved
src/main/resources/custom-events.sk Outdated Show resolved Hide resolved
src/main/resources/custom-events.sk Show resolved Hide resolved
src/main/java/ch/njol/skript/CustomEvents.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/CustomEvents.java Outdated Show resolved Hide resolved
@TheLimeGlass TheLimeGlass requested a review from sovdeeth August 21, 2023 20:44
Copy link
Member

@sovdeeth sovdeeth left a comment

Choose a reason for hiding this comment

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

Looks good, I think you may have missed this bit though (my fault, really):

You can leave the name blank, which doesn't seem to cause issues but should not be allowed imo:
image

@TheLimeGlass TheLimeGlass requested a review from sovdeeth August 21, 2023 22:19
src/main/resources/custom-events.sk Outdated Show resolved Hide resolved
@Moderocky Moderocky force-pushed the master branch 3 times, most recently from bd134d0 to 3f08853 Compare September 16, 2023 16:59
@sovdeeth sovdeeth changed the base branch from master to dev/feature December 30, 2023 07:53
@Moderocky Moderocky added feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. 2.9 Targeting a 2.9.X version release labels May 3, 2024
@sovdeeth sovdeeth removed feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. 2.9 Targeting a 2.9.X version release labels Jul 1, 2024
@APickledWalrus APickledWalrus added the up for debate When the decision is yet to be debated on the issue in question label Aug 16, 2024
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.

The implementation seems fine, but I do find myself wondering whether this is something we should add. We may be better off pointing these users to skript-reflect (which we now officially maintain) if they're looking for features like this. Up for debate.

@Moderocky Moderocky added the feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. label Aug 20, 2024
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. up for debate When the decision is yet to be debated on the issue in question
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants