Skip to content

Conversation

@rspforhp
Copy link

@rspforhp rspforhp commented Aug 3, 2025

Most of the changed code is commented with reasons why i changed it, the NEI is probably not the best implementation, but its working and i think its good

there were some issue related to porting from 1.12.2 i encountered while using MUI2, which i fix here
@rspforhp
Copy link
Author

rspforhp commented Aug 3, 2025

discord username: miya_kopie

@rspforhp
Copy link
Author

rspforhp commented Aug 3, 2025

i think the interface needs doc/comments, but i dont really know how to write those good

Copy link
Collaborator

@brachy84 brachy84 left a comment

Choose a reason for hiding this comment

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

Not sure about the NEI parts. You need to adjust the code style. Use a space after ìf` and around operators. The opening { goes on the same line as the method declaration. Avoid multiple empty lines. Always use { } in if statements if they are multiline. Use lower camel case for field names (at StackPositioner). You added line breaks to some places for no reason.

@rspforhp
Copy link
Author

rspforhp commented Aug 5, 2025

Not sure about the NEI parts. You need to adjust the code style. Use a space after ìf` and around operators. The opening { goes on the same line as the method declaration. Avoid multiple empty lines. Always use { } in if statements if they are multiline. Use lower camel case for field names (at StackPositioner). You added line breaks to some places for no reason.

I come from c#, so i forgot to follow the java coding style
i think other repos have spotless to help with that, but i think this one is missing it, i do not know why

@rspforhp
Copy link
Author

rspforhp commented Aug 5, 2025

Is this better?

@rspforhp rspforhp requested a review from brachy84 August 5, 2025 06:42
@brachy84
Copy link
Collaborator

brachy84 commented Aug 5, 2025

I come from c#, so i forgot to follow the java coding style i think other repos have spotless to help with that, but i think this one is missing it, i do not know why

It's easier to sync with upstream this way.

@brachy84
Copy link
Collaborator

brachy84 commented Aug 5, 2025

Is this better?

You barely did anything i asked for.

@rspforhp
Copy link
Author

rspforhp commented Aug 5, 2025

I come from c#, so i forgot to follow the java coding style i think other repos have spotless to help with that, but i think this one is missing it, i do not know why

It's easier to sync with upstream this way.

i see

@rspforhp
Copy link
Author

rspforhp commented Aug 5, 2025

Is this better?

You barely did anything i asked for.

i removed the line breaks, and put the { on the same line as the if, and used camel case, not sure what else there is? guess the spaces around operators?

@rspforhp rspforhp requested a review from brachy84 August 5, 2025 21:31
@Dream-Master
Copy link
Member

@brachy84 can you re check the pr? thanks

@Dream-Master Dream-Master requested a review from a team August 25, 2025 16:12
@rspforhp rspforhp requested a review from sisyphussy August 26, 2025 12:47
@rspforhp rspforhp requested a review from sisyphussy August 26, 2025 12:59
@sisyphussy
Copy link

I'll review in a couple hours once i'm on pc.

@rspforhp
Copy link
Author

yeah thats okay

@sisyphussy
Copy link

also, could you clarify what exactly this PR fixes so i can better justify the code and what it's doing?

@rspforhp
Copy link
Author

the fix for fluid slot was making sure it doesnt drain x fluid from fluid container if it doesnt have that amount
the container registry stuff was expected to return null on fail in a bunch of places, so i did that
crafting slot was always duping items on ground after finishing

@rspforhp rspforhp requested a review from sisyphussy August 27, 2025 17:59
@sisyphussy
Copy link

I haven't found any issues with the code, but this should probably get approved by a mui2 dev, since it does change some behaviour

@rspforhp rspforhp marked this pull request as draft August 29, 2025 08:13
Copy link
Collaborator

@brachy84 brachy84 left a comment

Choose a reason for hiding this comment

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

The mui part looks fine. No idea about the nei part

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.

4 participants