dizzyd / mcdex

Minecraft Modpack Management
Apache License 2.0
75 stars 8 forks source link

Improved MultiMC support #35

Closed mgziminsky closed 5 years ago

mgziminsky commented 5 years ago

Greatly improved the MultiMC functionality. This should also close #29, which is similar to what prompted me to make these changes. I'm considering also adding an mcdex tool integration into MultiMC to allow performing pack management directly from MultiMC.

dizzyd commented 5 years ago

I'm reviewing this, but have a couple of overarching concerns:

  1. I'd prefer updates to all deps to be a separate PR since we're going to need to do exhaustive testing to make sure nothing broke
  2. A number of the changes are stylistic in nature (i.e. changing variable names from things like gamePath to gameDir or changing indentation level on writing JSON - this clutters up the focus of the PR.

Before I do the review, please address these two problems so I can focus on the integration at hand. :)

mgziminsky commented 5 years ago

Ok.

  1. I originally updated them because I was having some issues setting up my dev environment which I thought was related to old deps, but it wasn't. I'll move these to another PR though since I didn't notice any issues.
  2. gameDir and modDir ins't a stylistic change, I changed the meaning of those values to more easily support the auto-name feature I added. The original *Path values now exist as methods. I can get rid of the json indentation change and other stylistic changes though. However, most of those changes were made due to my IDE showing them as warnings for not conforming to the standard conventions: https://github.com/golang/go/wiki/CodeReviewComments, so if you want, I can submit a code-cleanup PR addressing all such cases.