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.

Sign up for GitHub

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

Jump to bottom

Return API #6118

Merged
merged 24 commits into from
Jun 14, 2024
Merged

Return API #6118

merged 24 commits into from
Jun 14, 2024

Conversation

UnderscoreTud
Copy link
Member

Description

This PR adds an API to allow addons to easily use the return effect for their structures/sections without having to resort to hacky unintended methods to achieve the same effect

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

@UnderscoreTud UnderscoreTud added the feature Pull request adding a new feature. label Oct 12, 2023
@UnderscoreTud UnderscoreTud added the hacktoberfest-accepted Marks a PR as accepted for Hacktoberfest label Oct 13, 2023
TheLimeGlass
TheLimeGlass previously requested changes Oct 18, 2023
View reviewed changes
src/main/java/ch/njol/skript/effects/EffReturn.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/lang/TriggerSection.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/lang/TriggerSection.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/lang/TriggerSection.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/lang/TriggerSection.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/lang/TriggerSection.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/lang/parser/ParserInstance.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/lang/parser/ParserInstance.java Outdated Show resolved Hide resolved
@UnderscoreTud UnderscoreTud marked this pull request as draft December 25, 2023 20:25
@UnderscoreTud
Copy link
Member Author

Converting to draft. I don't really like how you interact with the API, may continue later

@TheLimeGlass TheLimeGlass mentioned this pull request Dec 27, 2023
Open
1 task
@UnderscoreTud
Merge branch 'dev/feature' into feature/return-api
8486ceb 
# Conflicts:
#	src/main/java/ch/njol/skript/lang/Section.java
#	src/main/java/ch/njol/skript/lang/TriggerSection.java
@UnderscoreTud
Add missing import
ad2f0b1
@UnderscoreTud
Only exit upto the returnable section when returning rather than the …
1413626 
…whole trigger
@UnderscoreTud
Requested Changes
911e5c9
@UnderscoreTud UnderscoreTud marked this pull request as ready for review May 10, 2024 14:43
sovdeeth
sovdeeth approved these changes May 10, 2024
View reviewed changes
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. Though I would prefer this to use parserinstance#Data, discussions on discord make it clear why it is not using that.

sovdeeth
sovdeeth requested changes May 14, 2024
View reviewed changes
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, do we have tests that cover this?

src/main/java/ch/njol/skript/lang/ReturnHandler.java Show resolved Hide resolved
src/main/java/ch/njol/skript/lang/ReturnHandler.java Outdated Show resolved Hide resolved
@UnderscoreTud UnderscoreTud requested a review from sovdeeth May 14, 2024 16:55
APickledWalrus
APickledWalrus requested changes May 15, 2024
View reviewed changes
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 work with taking the time to integrate this with less internal-integration :) Just a few things

src/main/java/ch/njol/skript/effects/EffReturn.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/lang/Section.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/lang/ReturnableTrigger.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/lang/ReturnableTrigger.java Show resolved Hide resolved
src/main/java/ch/njol/skript/lang/ReturnHandler.java Outdated Show resolved Hide resolved
@UnderscoreTud UnderscoreTud removed the request for review from TheLimeGlass June 11, 2024 11:56
@UnderscoreTud UnderscoreTud merged commit 1743000 into dev/feature Jun 14, 2024
6 checks passed
@UnderscoreTud UnderscoreTud deleted the feature/return-api branch June 14, 2024 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@APickledWalrus APickledWalrus APickledWalrus approved these changes

@Moderocky Moderocky Moderocky approved these changes

@sovdeeth sovdeeth sovdeeth approved these changes

@TheLimeGlass TheLimeGlass TheLimeGlass left review comments

Assignees
No one assigned
Labels
feature Pull request adding a new feature. hacktoberfest-accepted Marks a PR as accepted for Hacktoberfest
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@UnderscoreTud @sovdeeth @Moderocky @TheLimeGlass @APickledWalrus