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

Clarify CIP edge case behavior and add type safety guard #7363

Merged
merged 3 commits into from
Jan 15, 2025

Conversation

sovdeeth
Copy link
Member

@sovdeeth sovdeeth commented Jan 2, 2025

Description

Adds checks to Expression#changeInPlace() to guarantee that no invalid types will be passed to #change().
Improves the docs to clarify what happens in edge cases, and legitimizes the behavior of removing values if null is returned.
Also changes ChangerUtils#acceptsChange slightly by pre-fetching component types and spinning out the class comparisons to a separate method so changeInPlace can take advantage of the same code.


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

Also improves ChangerUtils#acceptsChange slightly.
@sovdeeth sovdeeth added the enhancement Feature request, an issue about something that could be improved, or a PR improving something. label Jan 2, 2025
@@ -50,7 +51,7 @@ public boolean supportsKeyedChange() {

abstract class ChangerUtils {

public static <T> void change(Changer<T> changer, Object[] what, Object @Nullable [] delta, ChangeMode mode) {
public static <T> void change(@NotNull Changer<T> changer, Object[] what, Object @Nullable [] delta, ChangeMode mode) {
Copy link
Member

Choose a reason for hiding this comment

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

What is the contract for what[]?

Copy link
Member Author

@sovdeeth sovdeeth Jan 2, 2025

Choose a reason for hiding this comment

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

idk i just clicked the intellij button to add notnull
I presume it's a notnull array with notnull values though

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.

Looks good. Is this still Internal API?

@sovdeeth
Copy link
Member Author

It should be safe for external use now but I would like to keep it internal a little longer, since I'm not 100% sure the design is flexible enough as is

@APickledWalrus APickledWalrus merged commit 529a5cb into SkriptLang:dev/feature Jan 15, 2025
5 checks passed
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants