pcal43 / quicksort

Minecraft mod. Low-effort item sorting to keep your workshop organized.
https://modrinth.com/mod/quicksort
MIT License
7 stars 5 forks source link

Add support for expanded storage #55

Closed ChristopherHaws closed 2 months ago

ChristopherHaws commented 2 months ago

Closes #54

pcal43 commented 2 months ago

Hey, thanks a lot for doing this. But a couple of things make me a little reluctant to merge it into main.

It seems like it would be a lot simpler to send a PR to expanded storage to make it implement the standard Container interface (which will allow it to work with quicksort)

pcal43 commented 2 months ago

...I guess this change also makes it so you can use expanded storage as a 'sorting chest' as well, which wouldn't be achieved by simply implementing container.

I guess it's still the same thing, though; I have a strong bias for keeping things simple, at least in terms of the releases I publish and maintain. But I'd be happy to merge this into a branch and let folks build their own releases from it if they want.

ChristopherHaws commented 2 months ago

Sorry about the formatting changes. I have my editor configured for work and didnt realize it was formatting the code till I had already finished with the implementation. I can go through and undo the formatting if you would like.

As for it making the code more complex, I don't see any way around this. I have a few mods I maintain and I have had to add compat in the past and this is how you do it. I could try and use pure FabricAPI events to try to simulate the logic, but it would probably be prone to errors since they don't have a specific event for opening and closing containers.

An alternative would be to add the two event's to ExpandedStorage's public API so that Quicksort wouldn't need to mixin ES's internal types, but there would still be the optional dependency on the mod for access to the API. I would be happy to see if they would accept a PR for adding the public API to their mod if you would feel more comfortable with this approach. This would be safer since you would be depending on their public api.

To be honest, I will probably need to maintain a fork for myself since I use this on my personal server which runs Create Mod which runs on 1.20.1 right now, so I am going to need to backport the code. I just figured I would contribute since you mentioned it was something others had asked for in the past. :)

pcal43 commented 2 months ago

Hey. Thanks a lot for the detailed and thoughtful response. I took another look.

I do think your refactor here to make things more event-based makes sense. And there are indeed some other folks who would benefit from the expandedstorage support (I'm just not sure how many).

My main thing is I just don't want to have to test and maintain the code against a growing list of other mods (especially when I don't even use those mods myself).

But I'd be happy to merge this as an unsupported feature (i.e., unsupported by me); I'd rely on you or other folks to keep it working. That would probably be easier for you than maintaining your own fork.

So, I think I will merge this into for now into a feature stabilization branch for now so I can clean up the whitespace diffs and a few other things, and then eventually merge it into main.

Thanks for the contribution!

ChristopherHaws commented 2 months ago

Hey. Thanks a lot for the detailed and thoughtful response. I took another look.

I do think your refactor here to make things more event-based makes sense. And there are indeed some other folks who would benefit from the expandedstorage support (I'm just not sure how many).

My main thing is I just don't want to have to test and maintain the code against a growing list of other mods (especially when I don't even use those mods myself).

But I'd be happy to merge this as an unsupported feature (i.e., unsupported by me); I'd rely on you or other folks to keep it working. That would probably be easier for you than maintaining your own fork.

So, I think I will merge this into for now into a feature stabilization branch for now so I can clean up the whitespace diffs and a few other things, and then eventually merge it into main.

Thanks for the contribution!

Awesome, thanks! Again, sorry about the formatting changes! I am usually the one yelling at people for changing the formatting in code ;)