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 5 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
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.

3 participants