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

Adding PredicateChoice to Paper API (updated version) #12017

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

CPieter
Copy link

@CPieter CPieter commented Jan 26, 2025

This PR is an updated version of derverdox's PredicateChoice PR #9996. Since the branch in the old PR was quite outdated I reïmplemented the changes in a new branch, but most of the changes remain the same. Credit of the code should go to derverdox and Machine-Maker.
The feature seems to work fine from my testing but I'm unfamiliar with the Paper API so I please let me know if I missed something.

@CPieter CPieter requested a review from a team as a code owner January 26, 2025 12:16
@CPieter CPieter changed the title Adding PredicateChoice to Paper API (second version) Adding PredicateChoice to Paper API (updated version) Jan 26, 2025
Copy link
Contributor

@lynxplay lynxplay left a comment

Choose a reason for hiding this comment

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

Welcome to paper 🎉
Thank you for updating the PR. I don'T have time for a indepth review rn, but I left some initial easy nitpicks around comments 👍

Will get to a review next week probably

@Doc94
Copy link
Contributor

Doc94 commented Jan 26, 2025

Hi and welcome to the Paper PR world (well this more a thing can say a Paper Team Member xd)

The paper comments currently just apply to code in patches, its not required in the api,craftbukkit,paper classes.

@CPieter
Copy link
Author

CPieter commented Jan 26, 2025

I just noticed there is already a PredicateRecipeChoice for potions, with the addition of the PredicateChoice this could be a bit confusing. Maybe it should be renamed to something more clear like a PotionRecipeChoice/PotionPredicateChoice?

@Doc94
Copy link
Contributor

Doc94 commented Jan 26, 2025

I just noticed there is already a PredicateRecipeChoice for potions, with the addition of the PredicateChoice this could be a bit confusing. Maybe it should be renamed to something more clear like a PotionRecipeChoice/PotionPredicateChoice?

well rename sounds a little breaking i think, maybe this new PredicateChoice can be named ItemPredicateChoice (? or add an extension of RecipeChoice for Potion and move things to that for make clear the diff... but not really sure.

@lynxplay lynxplay added type: feature Request for a new Feature. scope: api labels Jan 26, 2025
marked stackPredicate field in Ingredient as Nullable
Renamed field predicate to stackPredicate
@Machine-Maker
Copy link
Member

I just noticed there is already a PredicateRecipeChoice for potions, with the addition of the PredicateChoice this could be a bit confusing. Maybe it should be renamed to something more clear like a PotionRecipeChoice/PotionPredicateChoice?

Since PredicateRecipeChoice isn't public, delete it, and make PotionMix#createPredicateChoice return an instance of the new type. But also deprecate that method, and suggest to use the new method you create.

Here's a great example of what I talked about in my review, suggesting to keep implementation types out of the "api" because we can now just delete PredicateRecipeChoice entirely.

Added static constructor methods for MaterialChoice and ExactChoice to the RecipeChoice interface.
Deprecated old constructors.
Changed if to else-if, to prevent PredicateChoice recipe book examples from being added as exactIngredient
Copy link
Member

@Machine-Maker Machine-Maker left a comment

Choose a reason for hiding this comment

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

Looks good, just a few more things. I also opened the PR adding the future alternative to MaterialChoice. Will merge mine in after yours

@CPieter
Copy link
Author

CPieter commented Feb 3, 2025

Since PredicateRecipeChoice isn't public, delete it, and make PotionMix#createPredicateChoice return an instance of the new type. But also deprecate that method, and suggest to use the new method you create.

Here's a great example of what I talked about in my review, suggesting to keep implementation types out of the "api" because we can now just delete PredicateRecipeChoice entirely.

Most issues with the PR should now be resolved, but I'm not quite sure how to properly replace the PotionMix predicate choice. The new PredicateChoice requires an example stack, but this might not really make sense for use in the PotionMix. I'm hesitant to make the example stack nullable since not having one can break recipes. Maybe the PotionMix update should be left to a seperate PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: api type: feature Request for a new Feature.
Projects
Status: Changes required
Development

Successfully merging this pull request may close these issues.

4 participants