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

Loudly Failing 🗣️ Part: Effects #7417

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

Conversation

cheeezburga
Copy link
Member

Description

This PR implements SyntaxRuntimeErrorProducer on all relevant effects, with highlighting where possible and necessary, and generally cleans up/updates them to use modern conventions.

There were a few effects I left as is, as I thought they'd be overhauled sometime soon, or there was a significant recent PR changing them. Examples of these are: EffBan, EffVisualEffect, EffVehicle, etc.

I think the way these are implemented looks nice for the user, and displays relevant information, but opinions are more than welcome on this! I also think I changed standards half way through the PR, so I'll go back and update some of the first ones if there's no problems with the format.


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

@cheeezburga cheeezburga requested review from sovdeeth, APickledWalrus and Moderocky and removed request for sovdeeth January 9, 2025 09:54
@cheeezburga cheeezburga added housekeeping Housekeeping-related issue or task. enhancement Feature request, an issue about something that could be improved, or a PR improving something. labels Jan 9, 2025
Copy link
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.

the same type of changes apply to the rest of the classes as well. i recognize not set might be confused with not passing the expression. if you want, you can use <none>, since this is the null-eq of skript

src/main/java/ch/njol/skript/effects/EffCancelEvent.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffActionBar.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffApplyBoneMeal.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffBlockUpdate.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffCharge.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffColorItems.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffCancelDrops.java Outdated Show resolved Hide resolved
Copy link
Member

@Moderocky Moderocky left a comment

Choose a reason for hiding this comment

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

Do you think there might be a way to do the getnode stuff in a syntax superclass rather than in each one?

@TheAbsolutionism
Copy link
Contributor

Since you basically have almost all effects implementing SyntaxRuntimeErrorProducer, do you think it would be worth just making Effect.class implement it instead?

@cheeezburga
Copy link
Member Author

Do you think there might be a way to do the getnode stuff in a syntax superclass rather than in each one?

I was thinking this too, but I'm not entirely sure. This seems like kind of thing that @sovdeeth wouldve thought about while designing this system, so would love to hear his thoughts.

@sovdeeth
Copy link
Member

sovdeeth commented Jan 9, 2025

@TheAbsolutionism
Copy link
Contributor

@cheeezburga Just for clarity, Sovde was answering about my comment of implementing it straight into Effect.class and not about what you and Kenzie were referencing.

@sovdeeth
Copy link
Member

sovdeeth commented Jan 9, 2025

I'm talking about both of you

@cheeezburga cheeezburga requested a review from Moderocky January 11, 2025 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature request, an issue about something that could be improved, or a PR improving something. housekeeping Housekeeping-related issue or task.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants