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

Re-add support for itemtypes in CondIsOfType #5073

Merged
merged 8 commits into from
Aug 17, 2024

Conversation

Mr-Darth
Copy link
Contributor

Description

Looks like #1597 accidentally removed itemtypes support in CondIsOfType. This PR adds it back.


Target Minecraft Versions:
Requirements:
Related Issues:

@AyhamAl-Ali AyhamAl-Ali added the enhancement Feature request, an issue about something that could be improved, or a PR improving something. label Aug 30, 2022
@TheLimeGlass
Copy link
Collaborator

TheLimeGlass commented Sep 24, 2022

There is no entitytype to entitydata converter, this will break past compatability comparing an entity to an entitytype. Why did you remove entitytype?

Not all entitydata match 1:1 with entitytype. EntityData is based off Entity classes, it's designed to be used a literal and return a created entity. Entitytype will be removed in the future as Skript doesn't use Spigot EntityType enum, just not at this current time, so support for it is required.

@Mr-Darth
Copy link
Contributor Author

There is no entitytype to entitydata converter, this will break past compatability comparing an entity to an entitytype. Why did you remove entitytype?

Not all entitydata match 1:1 with entitytype. EntityData is based off Entity classes, it's designed to be used a literal and return a created entity. Entitytype will be removed in the future as Skript doesn't use Spigot EntityType enum, just not at this current time, so support for it is required.

From the looks of it, entitytype was never meant to be supported (and it isn't, only the pattern accepted it). And EntityType is just EntityData with an amount.

@TheLimeGlass TheLimeGlass added the needs testing Needs testing to determine current status or issue validity, or for WIP feature pulls. label Sep 28, 2022
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.

Might be worth looking into the EntityType thing as Lime mentioned

@TheLimeGlass TheLimeGlass self-requested a review December 16, 2022 00:09
@TheLimeGlass
Copy link
Collaborator

@Mr-Darth

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.

@Moderocky Moderocky changed the base branch from master to dev/feature September 18, 2023 09:37
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.

Some tests would be nice

@sovdeeth
Copy link
Member

Do you still intend to work on this?

@UnderscoreTud UnderscoreTud added bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. and removed enhancement Feature request, an issue about something that could be improved, or a PR improving something. labels Aug 16, 2024
@UnderscoreTud UnderscoreTud changed the base branch from dev/feature to dev/patch August 16, 2024 15:48
@sovdeeth sovdeeth added patch-ready A PR/issue that has been approved and is ready to be merged/closed for the next patch version. and removed needs testing Needs testing to determine current status or issue validity, or for WIP feature pulls. labels Aug 16, 2024
@sovdeeth sovdeeth added the 2.9 Targeting a 2.9.X version release label Aug 16, 2024
@UnderscoreTud UnderscoreTud merged commit d78bf61 into SkriptLang:dev/patch Aug 17, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.9 Targeting a 2.9.X version release bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. patch-ready A PR/issue that has been approved and is ready to be merged/closed for the next patch version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants