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

Adds some fire resistant syntax #6639

Merged
merged 15 commits into from
May 9, 2024

Conversation

cheeezburga
Copy link

@cheeezburga cheeezburga commented May 3, 2024

Description

Adds the new fire resistant stuff to Skript.

Currently, stuff that is already fire resistant (e.g. netherite boots, etc) won't respect these methods (i.e. you can't make stuff thats already fire resistant not fire resistant). This is a flaw in 1.20.6 that was missed, and should be fixed in 1.21 whenever that comes around. For now, I'll leave them out of the tests and add a note in the expression description detailing that.


Target Minecraft Versions: Minecraft 1.20.5
Requirements: Spigot 1.20.5
Related Issues: none

@cheeezburga cheeezburga requested a review from Moderocky May 3, 2024 16:47
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.

needs tests

Copy link
Contributor

@Fusezion Fusezion left a comment

Choose a reason for hiding this comment

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

Adding an expression to work along side others like %itemtype% named or %itemtype% with damage would be useful. Here's a mock design of one.

fire[ ]resistant %itemtypes%

give player fire resistant stone

%itemtypes% with fire[ ]resistance

give player stone with fire resistance

Adding format for in-verse effect would be useful too similar to how breakable player's tool is being added

- Added an expression to create a copy of an itemtype with or without fire resistance
- Added method checks to only register the syntax if the appropriate method exists
Copy link
Member

@AyhamAl-Ali AyhamAl-Ali left a comment

Choose a reason for hiding this comment

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

Other than Sovde's requests, LGTM.

@AyhamAl-Ali AyhamAl-Ali added the feature Pull request adding a new feature. label May 3, 2024
@cheeezburga
Copy link
Author

Adding an expression to work along side others like %itemtype% named or %itemtype% with damage would be useful. Here's a mock design of one.

Adding format for in-verse effect would be useful too similar to how breakable player's tool is being added

These should all be added 👍

needs tests

Added these too, but let me know if anything should change with them, first time writing tests.

Copy link
Contributor

@Fusezion Fusezion left a comment

Choose a reason for hiding this comment

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

Everything looks good from my view point

Copy link
Contributor

@ShaneBeee ShaneBeee left a comment

Choose a reason for hiding this comment

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

small little nit picks.

Co-authored-by: Shane Bee <shanebolenback@me.com>
- Happy to add back a different version of this like what Fuse suggested, but general consensus seemed to be to get rid of it
Co-authored-by: sovdee <10354869+sovdeeth@users.noreply.github.com>
@sovdeeth
Copy link
Member

sovdeeth commented May 4, 2024

re: test failures, it's when, not while. You should also be running the tests before you push your changes, it saves all of us time.

@cheeezburga cheeezburga requested a review from sovdeeth May 4, 2024 14:53
@sovdeeth sovdeeth added the feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. label May 8, 2024
@Moderocky Moderocky merged commit 524dc18 into SkriptLang:dev/feature May 9, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Pull request adding a new feature. feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants