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

Update to world events #5114

Merged

Conversation

Fusezion
Copy link
Contributor

@Fusezion Fusezion commented Sep 24, 2022

Description

This PR adds of %worlds% into the WorldSaveEvent, WorldLoadEvent, WorldUnloadEvent, and WorldInitEvent
I've also added a new effect for manually saving worlds

save all worlds
save [the] %worlds%

Target Minecraft Versions: any
Requirements: N/A
Related Issues: #5113 #3656

Removes world load/unload/init/save from simpleevents
Adds a save world effect
@Fusezion
Copy link
Contributor Author

Fusezion commented Sep 24, 2022

I haven't merged this as of yet as I'm unsure if it'll be worth it but should I also include an effect to
unload world as it exist within bukkit

I was planning to add an effect for load as well but with the fact Bukkit doesn't have a method for this I'm unable to

@TheLimeGlass
Copy link
Collaborator

I don't think you need world before %worlds% as the ClassInfo parse will remove the word world

@TheLimeGlass TheLimeGlass added enhancement Feature request, an issue about something that could be improved, or a PR improving something. feature Pull request adding a new feature. labels Sep 24, 2022
@Fusezion
Copy link
Contributor Author

Alright I'll remove it in a bit once I finish the breedable stuff

src/main/java/ch/njol/skript/effects/EffWorldUnload.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffSaveWorld.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffWorldUnload.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffWorldUnload.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffWorldUnload.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/events/EvtWorldUnload.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/events/EvtWorldUnload.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/events/EvtWorldUnload.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/events/EvtWorldUnload.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/events/EvtWorldUnload.java Outdated Show resolved Hide resolved
Fusezion and others added 3 commits September 29, 2022 06:07
This will be the first batch, anything not updated I'll edit manually on my end

Co-authored-by: LimeGlass <[email protected]>
Finished off the requested changes that weren't merged with first batch along with switch some other things around
Missed a requested change
Some requested changes
Syntax change for EffUnloadWorld
Changed from World[] to Literal<World>
Co-authored-by: LimeGlass <[email protected]>
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.

Nice additions :)

Instead of having four event classes though, I think you should just do something like https://github.com/SkriptLang/Skript/blob/master/src/main/java/ch/njol/skript/events/EvtItem.java where multiple events are registered in the same file (so that they use the same check method) since these check methods are very similar

src/main/java/ch/njol/skript/effects/EffWorldSave.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffWorldSave.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffWorldUnload.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffWorldUnload.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/events/EvtWorldLoad.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/events/EvtWorldUnload.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/events/EvtWorldUnload.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/events/EvtWorldSave.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/events/EvtWorldSave.java Outdated Show resolved Hide resolved
Combined all world events into a single file, "EvtWorld"
Did the requested changes for both effects
Added a warning about possibly freezing a server with the save effect

Co-Authored-By: Patrick Miller <[email protected]>
Removed some commented out code
Fixed a spelling mistake or two
Removed accidental * import
@Moderocky Moderocky changed the base branch from master to dev/feature September 18, 2023 09:42
@sovdeeth sovdeeth added 2.8 Targeting a 2.8.X version release feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. labels Dec 30, 2023
@sovdeeth sovdeeth removed the feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. label Dec 30, 2023
@Fusezion
Copy link
Contributor Author

Fusezion commented Jan 1, 2024

Latest commit remove the worlds plurality and the suggested getAnd error message. this event still works with plurals however only as on world save of "world" or "world_the_end if you attempt to use and skript will treat it as a list and error with the base one.

If a change is wanted to add it back I have zero intention to 'getAnd' calls on world save of "world" an and list when it should be false. Skript will need to change that behavior before this is attempted again.

@Fusezion Fusezion requested a review from sovdeeth January 1, 2024 05:54
@sovdeeth
Copy link
Member

sovdeeth commented Jan 1, 2024

Latest commit remove the worlds plurality and the suggested getAnd error message. this event still works with plurals however only as on world save of "world" or "world_the_end if you attempt to use and skript will treat it as a list and error with the base one.

If a change is wanted to add it back I have zero intention to 'getAnd' calls on world save of "world" an and list when it should be false. Skript will need to change that behavior before this is attempted again.

Sorry I'm really confused about what you're trying to say here
All I understood is that you fixed the issue with a single world failing?

@Fusezion
Copy link
Contributor Author

Fusezion commented Jan 1, 2024

Sorry I'm really confused about what you're trying to say here All I understood is that you fixed the issue with a single world failing?

Fixed? yes, lazily to be fair. I basically just removed it, Skript treats 1 or 2 the same as 1 so %world% supports both choices without 1 and 2 support. But I believe I might have miss understood your review, as I thought you were against and being a supported option

@Fusezion
Copy link
Contributor Author

Fusezion commented Jan 1, 2024

Alright now that I understood you weren't against and being supported, I've reverted previous commit and added your suggested of setAnd(false) let me know if I did it wrong

Orrr not give me a bit intellij is breaking

@Fusezion
Copy link
Contributor Author

Fusezion commented Jan 1, 2024

Alright so updated my branch and it broke it as setAnd was renamed in #6000 to invertAnd

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.

One small change

src/main/java/ch/njol/skript/events/EvtWorld.java Outdated Show resolved Hide resolved
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.

just some concerns about syntax consistency

src/main/java/ch/njol/skript/effects/EffWorldLoad.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffWorldLoad.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffWorldSave.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffWorldSave.java Outdated Show resolved Hide resolved
@APickledWalrus APickledWalrus merged commit 16e217d into SkriptLang:dev/feature Jan 1, 2024
4 checks passed
@Fusezion Fusezion deleted the enhancment/worldupdates branch January 1, 2024 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.8 Targeting a 2.8.X version release enhancement Feature request, an issue about something that could be improved, or a PR improving something. feature Pull request adding a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants