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

Add ingredient serialiser for list ingredients #792

Open
wants to merge 5 commits into
base: 1.21
Choose a base branch
from

Conversation

Abbie5
Copy link
Contributor

@Abbie5 Abbie5 commented Nov 21, 2024

No description provided.

@emilyploszaj
Copy link
Owner

This looks really reasonable, I'm wondering if there's any benefit to not serializing list ingredients? For example, EMI list ingredients are basically a fallback, and it'd be preferable if resource driven recipes and such only could add tag ingredients. Also, shouldn't this attempt to coerce ingredient lists to tags? Maybe lists of stacks for ingredient accepting values should be natively supported and coerced through several heuristics?

@Abbie5
Copy link
Contributor Author

Abbie5 commented Dec 8, 2024

Hmm, I was operating on the assumption that deserialising then reserialising should ideally return the input (although to do that properly would require making ListEmiIngredient#ingredients readable somehow, probably through a getter). This behaviour can be changed easily though, just by using EmiIngredient::of in place of new ListEmiIngredient

IMO, list ingredients should be able to be defined in resource-driven recipes, as tags cannot be added client-side.

I was a bit afraid of accidentally changing existing behaviour by interpreting lists of stacks as list ingredients, so I left it out of the PR. It can be added, though.

@Abbie5
Copy link
Contributor Author

Abbie5 commented Dec 23, 2024

implemented the changes i mentioned, sorry for the long delay. tested in both world interaction and info recipes, as well as favouriting list ingredients.

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.

2 participants