maradotwebp / pax

📦 The MC modpack manager for professionals.
https://github.com/froehlichA/pax/releases/latest
MIT License
164 stars 3 forks source link

Adds auto dependency installation and removal #38

Closed Badtz13 closed 3 years ago

Badtz13 commented 3 years ago

This pull request closes #8, which referenced a --dependencies flag to remove dependencies. However, we found that it was a better experience to simply ask the user if they wanted to remove dependencies if they exist. Feel free to revert the most recent commit if you think otherwise.

You don't have any contribution guidelines, but this is a cool project and we wanted to help out. Sorry if this is unwanted.

maradotwebp commented 3 years ago

Nice! Contributions aren't unwanted at all :D I simply haven't gotten around to contributions guidelines yet. I'll be reviewing your PR shortly.

langedev commented 3 years ago

Summary of changes: The add command now adds the name of the mod, whether or not it was explicitly installed, and a list of dependency ids to the manifest. This required passing a lot more data to installMod, so we opted to have it take a ManifestFile datatype as its arg. (additionally we made it so removeMod "popped" the ManifestFile, returning it, as this allows one to remove the mod, and then inform the user it was removed by name)

Additionally we now recursively add and remove mod dependencies. For instance, adding https://www.curseforge.com/minecraft/mc-mods/curios-quark-oddities-backpack adds Curios, Quark, and Quark Oddities, and since Quark depends on AutoRegLib it will also be installed. Removing is similarly recursive, though it only checks the manifest file, and doesn't have to query the website.

A note on how we wrote to the manifest. We opted to write directly wrather than in a __paxmeta wrapper. putting it in this way promotes portability, since there is no pax branding so another modpack builder could use the data. Improves readability, and seemed to be a simpler option.

Something to be considered: There is currently no way to mark a mod that was installed implicitly as explicit. If I install Just Enough Resources, then Just Enough Items is installed, and there is no way to then mark Just Enough Items as being explicitly installed. Seems like a seperate issue, but I wanted to mention it here.

maradotwebp commented 3 years ago

We opted to write directly wrather than in a __paxmeta wrapper. putting it in this way promotes portability, since there is no pax branding so another modpack builder could use the data. Improves readability, and seemed to be a simpler option.

Makes sense, but I'd still advocate for at least putting them into a "__meta": { ... } object. It's a bit more lines in the manifest, but it feels cleaner to me and we can more easily add a feature to ./pax export that would purge those.

Something to be considered: There is currently no way to mark a mod that was installed implicitly as explicit. If I install Just Enough Resources, then Just Enough Items is installed, and there is no way to then mark Just Enough Items as being explicitly installed. Seems like a seperate issue, but I wanted to mention it here.

Two possibilities:

Badtz13 commented 3 years ago

We implemented all the changes you requested, including moving the metadata to a __meta subtag. Importing now also generates metadata automatically. removeDependencies now recursively checks to make sure no dependencies are removed.

We have yet to implement a clean export method (--purge), but this seemed quite difficult, and most likely would be better addressed in another issue/pull request.

maradotwebp commented 3 years ago

Yeah, clean export is for a story for another Pull Request.

maradotwebp commented 3 years ago

Last change - Otherwise, this looks good to me! Again, thanks for contributing - I didn't think I'd get much pull requests with a language like Nim :P

langedev commented 3 years ago

Haha, didn't think I'd learn nim. It's a pretty nice language, as someone who likes C but has a hard time with python. Really enjoying pax, thanks for creating it.