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.

Sign up for GitHub

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

Jump to bottom

Recipe Remainder API #158

Merged
merged 34 commits into from Oct 8, 2022
Merged

Recipe Remainder API #158

merged 34 commits into from Oct 8, 2022

Conversation

Platymemo
Copy link
Contributor

As per @OroArmor having to drop #82, I will be picking it up.

An extensive, no missed location, API for recipe remainders.

Any crafting location has its location checked for remainders and carefully implemented. The impl also makes it easy for users to add this into their own code

Leo40Git
Leo40Git reviewed Jul 31, 2022
View reviewed changes
Copy link
Contributor

@Leo40Git Leo40Git left a comment

Choose a reason for hiding this comment

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

A few comments.

...item/item_setting/src/main/java/org/quiltmc/qsl/item/setting/mixin/SimpleInventoryMixin.java Outdated Show resolved Hide resolved
library/item/item_setting/src/main/java/org/quiltmc/qsl/item/setting/api/QuiltItemSettings.java Outdated Show resolved Hide resolved
EnnuiL
EnnuiL suggested changes Aug 6, 2022
View reviewed changes
Copy link
Contributor

@EnnuiL EnnuiL left a comment

Choose a reason for hiding this comment

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

oh god, i saw something terrible
Here's my one cent, the rest will come later

library/core/qsl_base/src/main/java/org/quiltmc/qsl/base/mixin/SharedConstantsMixin.java Outdated Show resolved Hide resolved
@EnnuiL EnnuiL added enhancement New feature or request library: item Related to the item library. t: new api This adds a new API. s: wip This pull request is being worked on. labels Aug 7, 2022
Leo40Git
Leo40Git approved these changes Aug 7, 2022
View reviewed changes
Copy link
Contributor

@Leo40Git Leo40Git left a comment

Choose a reason for hiding this comment

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

LGTM!

...tem/item_setting/src/main/java/org/quiltmc/qsl/item/setting/api/QuiltCustomItemSettings.java Show resolved Hide resolved
@Platymemo Platymemo marked this pull request as ready for review September 3, 2022 00:17
@Platymemo Platymemo requested a review from a team as a code owner September 3, 2022 00:17
Leo40Git
Leo40Git approved these changes Sep 8, 2022
View reviewed changes
Copy link
Contributor

@Leo40Git Leo40Git left a comment

Choose a reason for hiding this comment

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

LGTM!

...item_setting/src/main/java/org/quiltmc/qsl/item/setting/api/RecipeRemainderLogicHandler.java Outdated Show resolved Hide resolved
@Platymemo @Leo40Git
apply suggestion
a73ff10 
Co-authored-by: ADudeCalledLeo <7997354+Leo40Git@users.noreply.github.com>
@EnnuiL EnnuiL added s: waiting for test This pull request is waiting to be tested, the PR cannot be put in FCP until it has been tested. and removed s: wip This pull request is being worked on. labels Sep 11, 2022
EnnuiL
EnnuiL suggested changes Sep 11, 2022
View reviewed changes
Copy link
Contributor

@EnnuiL EnnuiL left a comment

Choose a reason for hiding this comment

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

Everything seems fine here! Just do these changes:

...java/org/quiltmc/qsl/item/setting/mixin/reciperemainder/AbstractFurnaceBlockEntityMixin.java Outdated Show resolved Hide resolved
library/item/item_setting/src/main/resources/quilt_item_setting.mixins.json Outdated Show resolved Hide resolved
@Leo40Git Leo40Git mentioned this pull request Sep 26, 2022
Open
EnnuiL
EnnuiL suggested changes Sep 28, 2022
View reviewed changes
Copy link
Contributor

@EnnuiL EnnuiL left a comment

Choose a reason for hiding this comment

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

I'll probably want a Checkstyle pass on the whole API if there wasn't one already; After that? Yeah, we can approve!

@EnnuiL EnnuiL added s: tested This pull request has been tested and confirmed as working. and removed s: waiting for test This pull request is waiting to be tested, the PR cannot be put in FCP until it has been tested. labels Sep 29, 2022
LemmaEOF
LemmaEOF suggested changes Sep 30, 2022
View reviewed changes
Copy link
Contributor

@LemmaEOF LemmaEOF left a comment

Choose a reason for hiding this comment

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

This is not blocking, but I really don't like the names of the new settings - they feel clunky and un-ergonomic, so I've suggested some potential other names. I can live with 'em if they're what we've gotta use, but I feel like they can be better.

library/item/item_setting/src/main/java/org/quiltmc/qsl/item/setting/api/QuiltItemSettings.java Outdated Show resolved Hide resolved
library/item/item_setting/src/main/java/org/quiltmc/qsl/item/setting/api/QuiltItemSettings.java Outdated Show resolved Hide resolved
library/item/item_setting/src/main/java/org/quiltmc/qsl/item/setting/api/QuiltItemSettings.java Outdated Show resolved Hide resolved
@hibiii hibiii mentioned this pull request Sep 30, 2022
Open
@EnnuiL EnnuiL removed the enhancement New feature or request label Sep 30, 2022
EnnuiL
EnnuiL suggested changes Oct 5, 2022
View reviewed changes
Copy link
Contributor

@EnnuiL EnnuiL left a comment

Choose a reason for hiding this comment

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

Few javadoc changes that I suggest;

Also, while Lemma did said that its review isn't blocking, I do wanna have it at least considered; We still can afford method renames later on, but yeah, better change now than late

...setting/src/main/java/org/quiltmc/qsl/item/setting/impl/RecipeRemainderLogicHandlerImpl.java Outdated Show resolved Hide resolved
...setting/src/main/java/org/quiltmc/qsl/item/setting/impl/RecipeRemainderLogicHandlerImpl.java Outdated Show resolved Hide resolved
...setting/src/main/java/org/quiltmc/qsl/item/setting/impl/RecipeRemainderLogicHandlerImpl.java Outdated Show resolved Hide resolved
Platymemo and others added 2 commits October 5, 2022 14:27
@Platymemo @EnnuiL
Apply suggestions
f6f0f40 
Co-authored-by: Ennui Langeweile <85590273+EnnuiL@users.noreply.github.com>
@Platymemo
update recipe remainder names
1ace64d
@Platymemo Platymemo requested a review from EnnuiL October 6, 2022 23:09
@EnnuiL EnnuiL merged commit 9cddb31 into QuiltMC:1.19 Oct 8, 2022
@magneticflux- magneticflux- mentioned this pull request Nov 20, 2022
Merged
@Platymemo Platymemo deleted the recipe-remainder-api branch April 4, 2023 04:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LemmaEOF LemmaEOF LemmaEOF approved these changes

@Leo40Git Leo40Git Leo40Git approved these changes

@OroArmor OroArmor OroArmor approved these changes

@EnnuiL EnnuiL EnnuiL approved these changes

Assignees
No one assigned
Labels
final-comment-period library: item Related to the item library. s: tested This pull request has been tested and confirmed as working. t: new api This adds a new API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Platymemo @LemmaEOF @Leo40Git @OroArmor @EnnuiL