SpongePowered / SpongeAPI

A Minecraft plugin API
http://www.spongepowered.org/
MIT License
1.14k stars 342 forks source link

Abstract ItemStack & ItemStackSnapshot to ItemStackLike #2563

Closed MrHell228 closed 1 month ago

MrHell228 commented 1 month ago

SpongeAPI | Sponge

Completely QOL PR. I was doing some item stuff and realized I need a lot of copy-pasted methods for ItemStack and ItemStackSnapshot. Then I realized they are in fact the same object with the only difference is mutability, so why not abstract it?

Also made ItemStack#maxStackQuantity() defaulted in ItemStackLike (this PR allowed it).

Breaking changes:

These breaking changes are not vital for this PR so if they are not okay, I can revert them. But they feel natural to work with ItemStackLike (and imo new methods just look better).

Possible usage in API (I guess this makes sense only if breaking changes are accepted): There are a lot of places where methods accept only ItemStack or only ItemStackSnapshot while there is no real difference for dev in what to pass (because ItemStacks are copied anyway). So they can be easily changed to accept ItemStackLike

gabizou commented 1 month ago

I wouldn't mind keeping the "old" methods but marking them deprecated. If we keep the old methods, I'm also happy to start adding overload methods that accept ItemStackLike over ItemStack, likewise being deprecated for removal. I suppose that a lot of the methods that are generic based on ItemStack would not be migrated since that'd be casting incompatible.

MrHell228 commented 1 month ago

Added old methods back as defaulted with deprecation.

I'm not sure what do you mean with deprecating methods that accept ItemStack. Won't they just be fine in API with simple ItemStack -> ItemStackLike replace? I mean, nothing would break since ItemStack extends ItemStackLike.

About generic methods based on ItemStack: There are not that much such things in API (found some in recipe building) and it's pretty easy to convert them. Unless you mean they should not be migrated yet because it would be breaking change.

Also this PR seems to fail on build stage due to reasons I don't know and don't think that's because of me.

aromaa commented 1 month ago

Won't they just be fine in API with simple ItemStack -> ItemStackLike replace? I mean, nothing would break since ItemStack extends ItemStackLike.

This is a binary breaking change. When plugins are compiled they target specific method signature, which the JVM uses to distinguish different methods at runtime. By changing the method parameters you change the signature which invalidates the old signature.

MrHell228 commented 1 month ago

Ah, got it. Didn't know it works this way

MrHell228 commented 1 month ago

Migrated whole item package (and few things outside of it that I found) to use ItemStackLike. Old methods are now defaulted to use ItemStackLike variant and deprecated for removal.

There were some generic based methods in recipes that I changed directly:

I guess that's not breaking change since it has the same signature and is casting compatible with the old method.

So, assuming changes above are fine, I think this PR is ready.