search

iChun / iChunUtil

iChunUtil as required by several of my mods.
GNU Lesser General Public License v3.0
40 stars 53 forks source link

Updates & Fixes (See #169 & #168) #171

Closed CDAGaming closed 6 years ago

CDAGaming commented 6 years ago

Referencing #169 and #168, This PR includes Removing Deprecated Methods as well as a Temporary workaround to allow IChunUtil & mods requiring it to run on Forge 14.23.2.2622 and Upwards

This PR also Includes Ignoring other IDE Files as well

iChun commented 6 years ago

The issue you attempted to fix was fixed by Forge, but I do have a few comments, looking at your PR.

  1. At some point I really should add the Eclipse classes/folders to my .gitignore. I use IDEA so my gitignores are set up for it (clearly)
  2. You shouldn't be copying morph's source into /src/main/java/ to compile the mod, that's what /src/api/ is for.
  3. My build.gradle has typos because it's practically a copy and edit of Forge's old build.gradle... Lex isn't always the best speller. They're just comments though so I never really bothered fixing them.
  4. All your changes from I18n to TextComponentTranslation weren't necessary. MC has two I18n classes, only one if found on the server and is the one I use.
  5. Some quick lookup told me that your use of StringBuilder was justified, it's performance in iteration is much better than concatenating the String, but the performance improvement is only significant with a large number of iterations. I guess I just found it quicker/easier to concat the string rather than use a String builer at the time I wrote that code.
  6. What you did that actually "fixed" the mod running on Forge 2623 was changing the mod's dependencies so that it could only run on Forge 2623, instead of a range of Forge builds. I'm sure you understand how this is a big no no.
  7. Protip: Some devs won't mind multiple commits in a PR, but some do. Try looking into squashing your commits next time as well.

That said though, I appreciate your efforts in attempting to fix this! But as mentioned the issue was in Forge and they have fixed it their end so I will be closing this PR.