Skip to content

Yeet reflective item parsing#75

Merged
UltraProdigy merged 17 commits intomasterfrom
yeet-itemparse-v2
Jan 28, 2026
Merged

Yeet reflective item parsing#75
UltraProdigy merged 17 commits intomasterfrom
yeet-itemparse-v2

Conversation

@ah-OOG-ah
Copy link
Member

By default, Binnie reflects over itself and all the child mods to find and register items. This is pointless and ugly, so I replaced it with explicitly initializing items.

@mts2200
Copy link

mts2200 commented Nov 1, 2025

You break all addons because change public signature of items & blocks

@Dream-Master Dream-Master requested a review from a team November 1, 2025 23:03
@ah-OOG-ah
Copy link
Member Author

You break all addons because change public signature of items & blocks

Which addons?

@boubou19
Copy link
Member

You break all addons because change public signature of items & blocks

it's kind of needed because binnie is known for being trash code.

@boubou19
Copy link
Member

Also, it seems we don't want to support our fork of binnies outside of NH according to our supported spreadsheet, so it's a non issue

@Dream-Master Dream-Master added the 🚧 Testing on Zeta Do not merge yet, testing this PR on Zeta label Dec 20, 2025
Copy link

@Nikolay-Sitnikov Nikolay-Sitnikov left a comment

Choose a reason for hiding this comment

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

If you register items in the constructor, it makes it impossible to extend those items in addons. It also means that if the class is ever instantiated elsewhere, it could crash because the wrong/no mod container will be active.

I recommend either something along the lines of the item enum used in Utilities In Excess or something along those lines.

@ah-OOG-ah
Copy link
Member Author

If you register items in the constructor, it makes it impossible to extend those items in addons. It also means that if the class is ever instantiated elsewhere, it could crash because the wrong/no mod container will be active.

I recommend either something along the lines of the item enum used in Utilities In Excess or something along those lines.

Why? Binnie itself is extending these items, and it works just fine.

@Nikolay-Sitnikov
Copy link

If you register items in the constructor, it makes it impossible to extend those items in addons. It also means that if the class is ever instantiated elsewhere, it could crash because the wrong/no mod container will be active.
I recommend either something along the lines of the item enum used in Utilities In Excess or something along those lines.

Why? Binnie itself is extending these items, and it works just fine.

That shouldn't work. You're registering multiple items for the same ID. If it works, I have no idea how.

@ah-OOG-ah
Copy link
Member Author

That shouldn't work. You're registering multiple items for the same ID. If it works, I have no idea how.

...what? It's using the unlocalized name as part of the ID, it works just fine. I could add a constructor param allowing you to override that name without Java 25 (flexible constructor bodies), but I don't think it's important.

From what I can tell, there aren't any addons for Binnie's, except for a private one by mts2000. It doesn't appear to be on his GitHub, and I can't find it on Modrinth or Curse, so I can't test it for compat regardless. I'm not going to attempt support for completely unknown and inaccessible codebases.

Copy link

@sisyphussy sisyphussy 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 to me, though nick requested changes so you'll need to wait for him to look at it again. Since there haven't been any issues made for it in zeta, I don't think there's much that can go wrong here. :cope:

@Nikolay-Sitnikov
Copy link

I see the cleverness of the design and why it works, although I still think it's not a good design. (Having a constructor call a virtual method that subclasses override is generally a recipe for strange behavior, but it's fine in this case.) It's better than what we have now, so go ahead and merge it.

@Nikolay-Sitnikov Nikolay-Sitnikov dismissed their stale review January 28, 2026 03:12

See above comment.

@UltraProdigy UltraProdigy merged commit 6ba6612 into master Jan 28, 2026
1 check passed
@UltraProdigy UltraProdigy deleted the yeet-itemparse-v2 branch January 28, 2026 03:29
@UltraProdigy UltraProdigy removed the 🚧 Testing on Zeta Do not merge yet, testing this PR on Zeta label Jan 28, 2026
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.

7 participants

Comments