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

Abstract ItemStack & ItemStackSnapshot to ItemStackLike #2563

Merged
merged 11 commits into from
Sep 28, 2024

Conversation

MrHell228
Copy link
Contributor

@MrHell228 MrHell228 commented Sep 10, 2024

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:

  • Removed ItemStack#createSnapshot() and ItemStackSnapshot#createStack()
  • Added ItemStackLike#asMutable(), #asMutableCopy() and #asImmutable()

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
Copy link
Member

gabizou commented Sep 13, 2024

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
Copy link
Contributor Author

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
Copy link
Member

aromaa commented Sep 13, 2024

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
Copy link
Contributor Author

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

@MrHell228
Copy link
Contributor Author

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:

  • Function<RecipeInput.Crafting, ItemStack> -> Function<RecipeInput.Crafting, ? extends ItemStackLike> (in SpecialCraftingRecipe)
  • Function<RecipeInput.Crafting, List<ItemStack>> -> Function<RecipeInput.Crafting, ? extends List<? extends ItemStackLike>> (remainingItems methods)

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.

@gabizou gabizou self-requested a review September 28, 2024 21:11
@gabizou gabizou merged commit c8b38af into SpongePowered:api-12 Sep 28, 2024
2 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants