team-abnormals / blueprint

Library that implements the framework of all Abnormals mods
https://www.curseforge.com/minecraft/mc-mods/blueprint
Other
114 stars 33 forks source link

[1.16.3] Adds overloads of the compat methods to allow for two mod id's #33

Closed EltrutLikes closed 3 years ago

EltrutLikes commented 3 years ago

This PR adds overloads of the compat methods to allow for compatibility between two different mods. This is used, for example, in Enhanced Mushrooms for glowshroom beehives or in Cookielicious for sweet berry cookie tile vertical slabs. A common use of these methods is for allowing compatibility between some mod and Quark's vertical slabs and is used often enough that I think it warrants a PR here.

EltrutLikes commented 3 years ago

I've fixed the documentation issues and changed the methods to take in an ArrayList of mod id's and a boolean isIndev instead of two mod id's. I also added a compat method for the chests as well just for completeness and I switched the parameters of the original chest compat method to match the other compat methods. Let me know if you would like me to revert that though. :)

jacksonhardaway commented 3 years ago

Would probably be best to use a String varargs instead of an ArrayList, since you don't need to keep the order intact plus it'll just be cleaner for developers to put in multiple strings.

Also, instead of using an isIndev boolean, it'd probably be better to just check if the environment is a deobf environment.

EltrutLikes commented 3 years ago

Apologies for the noobiness, I'm still learning Java.

A question though: if I switch it over to varargs, would you like me to combine the multi-mod compat method, one-mod compat method, and normal method into one (since one-mod and normal would correspond to 1 and 0 parameters, respectively) or just leave them as is?

EltrutLikes commented 3 years ago

I switched over to a String varargs for the mod id list, removed isIndev as a parameter in favor of just checking whether it is a dev environment or not, moved the imports back to where they were, and combined the multi-mod and single mod compat methods.

I feel that it is now redundant to have for example both a createBlock and createCompatBlock method with the String varargs since createBlock just corresponds to the createCompatBlock with no parameters, but I understand if for clarity you would like to keep both.

bageldotjpg commented 3 years ago

Ok, sorry for changing up things yet again but Jackson, Smelly and I were discussing in VC yesterday and determined some things.

We also determined the change to var args, while easy to use, is rather unclean in a development format, and forcing it upon all methods is not a great idea, because that creates a massive breaking change that is also not very readable. So the vararg methods should be new methods and leave the old ones alone, since they are rarely used (they've never been used by us) and not worth creating a big breaking change over.

EltrutLikes commented 3 years ago

I restored the original compat methods and removed the if (FMLEnvironment.production) conditional from the multiple compat methods (which are now once again overloads of the original methods). Hopefully third time is a charm :)

SmellyModder commented 3 years ago

I updated the 1.16.3 branch to 1.16.4 so this got closed. Open a new PR for this on the 1.16.4 branch.