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

Structure API Finalization #5669

Conversation

APickledWalrus
Copy link
Member

@APickledWalrus APickledWalrus commented May 3, 2023

Description

This PR aims to make the final changes before the Structure API is "officially released" with 2.7

Changes:

  • Script Event/Data methods are now marked as Experimental as they are subject to change in 2.8
  • Removed event parameters from the utility EntryData classes
    • developers should instead opt to manually set the current event in their Structure's load method (this is also to remove Bukkit dependency in these files)
  • Fixed an issue where scripts could no longer have multiple options sections
  • Priority is now considered for all Structure loading methods
  • Fixed an issue with a deprecated reload method not replicating the same behavior
  • Improved the EntryContainer get methods so that additional casting is not required for classes like List

If there any other suggested changes we want to make, let me know!


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

These methods will be subject to change in the future (e.g. EventRegister API/PR)
Developers should instead opt to manually set the current event within their Structure load methods.
This was actually not necessarily and reduces confusion/complexity in the methods.
@APickledWalrus APickledWalrus added enhancement Feature request, an issue about something that could be improved, or a PR improving something. 2.7 Targeting a 2.7.X version release breaking changes Pull or feature requests that contain breaking changes (API, syntax, etc.) labels May 3, 2023
apparently the parser doesn't do this :(
@TheLimeGlass
Copy link
Collaborator

TheLimeGlass commented May 4, 2023

Removal of the EntryContainer class types is just guess work. You could use TypeToken to ensure the type matches the generic. I don't see the point of unsafe type casting when it could just be returning an Object and it's up to the developer to handle proper casting, rather than Skript assuming the cast would work. This is bad practice.

Everything else is fine.

@TheLimeGlass TheLimeGlass requested a review from Moderocky May 4, 2023 21:27
@APickledWalrus
Copy link
Member Author

The main idea was to simplify how much needed to be done to just obtain the entries. e.g. it's much simpler for the developer to just be able to do: boolean myEntry = entryContainer.getOptional("myEntry", false); (especially for cases where you might have maps, but this change is certainly debatable)

I thought the constant specification of the type was more of an annoyance than anything, but I'd certainly like to hear other opinions :))

@TheLimeGlass
Copy link
Collaborator

The main idea was to simplify how much needed to be done to just obtain the entries. e.g. it's much simpler for the developer to just be able to do: boolean myEntry = entryContainer.getOptional("myEntry", false); (especially for cases where you might have maps, but this change is certainly debatable)

I thought the constant specification of the type was more of an annoyance than anything, but I'd certainly like to hear other opinions :))

Ya I don't agree with that. Let the developer cast it rather than assumption.

@TheLimeGlass TheLimeGlass self-requested a review May 17, 2023 02:00
@TheLimeGlass TheLimeGlass requested a review from AyhamAl-Ali May 31, 2023 09:34
@TheLimeGlass TheLimeGlass marked this pull request as draft June 3, 2023 05:37
Copy link
Collaborator

@TheLimeGlass TheLimeGlass left a comment

Choose a reason for hiding this comment

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

My changes above and also needs to target dev/2.7 now as discussed in Discord.

TO ALL SKRIPTLANG DEVS: Please don't update this pull request to master, until Pickle changes the target branch to dev/2.7 to avoid making Pickle do revisions.

@Moderocky Moderocky marked this pull request as ready for review July 12, 2023 08:11
* Adds new ScriptData to this Script's data map.
* @param data The data to add.
*/
@ApiStatus.Experimental
Copy link
Member

Choose a reason for hiding this comment

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

If you expect these methods to change it would probably have been better to put them in some kind of 'unsafe' class, rather than putting warnings on these.

Copy link
Member Author

Choose a reason for hiding this comment

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

These are expected to change in #5552 which is now targeting 2.8 (hence these annotations)

I'm not expecting a big impact (if any) on developers

This is a breaking change (within 2.7, these methods did not exist in 2.6)
@APickledWalrus APickledWalrus added the bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. label Aug 29, 2023
This restores the behavior of the method loading the script even if it wasn't previously loaded.
With some generic changes to prevent additional casting for users of generic classes like List
@APickledWalrus APickledWalrus merged commit 76db944 into SkriptLang:master Sep 7, 2023
@APickledWalrus APickledWalrus deleted the feature/structure-api-finalization branch September 7, 2023 17:52
AyhamAl-Ali pushed a commit to AyhamAl-Ali/Skript that referenced this pull request Sep 9, 2023
Moderocky pushed a commit that referenced this pull request Sep 9, 2023
Structure API Finalization (#5669)

Co-authored-by: Patrick Miller <apickledwalrus@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.7 Targeting a 2.7.X version release breaking changes Pull or feature requests that contain breaking changes (API, syntax, etc.) bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. 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.

5 participants