Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds auto dependency installation and removal #38

Merged
merged 27 commits into from
Jul 7, 2021

Conversation

Badtz13
Copy link
Contributor

@Badtz13 Badtz13 commented Jul 6, 2021

  • Add command now installs dependencies (requires user approval)
    • Added test to make sure dependencies are found
  • Remove command removes dependencies (also requires approval)

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
Copy link
Owner

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.

src/api/cf.nim Outdated Show resolved Hide resolved
src/api/cf.nim Outdated Show resolved Hide resolved
src/cmd/add.nim Outdated Show resolved Hide resolved
src/cmd/add.nim Outdated Show resolved Hide resolved
src/api/cf.nim Outdated Show resolved Hide resolved
src/api/cf.nim Outdated Show resolved Hide resolved
src/api/cf.nim Outdated Show resolved Hide resolved
src/cmd/add.nim Outdated Show resolved Hide resolved
src/pax.nim Show resolved Hide resolved
src/cmd/remove.nim Outdated Show resolved Hide resolved
PanDoes and others added 5 commits July 6, 2021 16:38
@Badtz13 Badtz13 requested a review from maradotwebp July 7, 2021 07:45
@langedev
Copy link
Contributor

langedev commented Jul 7, 2021

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.

Copy link
Owner

@maradotwebp maradotwebp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Just a few changes, but I think that should do the job. :D
Hope I didn't forget any edge cases with removing mods..

src/api/cf.nim Outdated Show resolved Hide resolved
src/cmd/add.nim Outdated Show resolved Hide resolved
src/cmd/add.nim Outdated Show resolved Hide resolved
src/modpack/files.nim Outdated Show resolved Hide resolved
src/modpack/files.nim Outdated Show resolved Hide resolved
src/cmd/remove.nim Outdated Show resolved Hide resolved
src/cmd/add.nim Outdated Show resolved Hide resolved
src/cmd/remove.nim Outdated Show resolved Hide resolved
src/cmd/remove.nim Outdated Show resolved Hide resolved
tests/modpack/tfiles.nim Outdated Show resolved Hide resolved
@maradotwebp
Copy link
Owner

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:

  • Pax currently prevents already installed mods from being installed with ./pax add again due to Remove duplicates when installing #36. Instead of erroring if a user tries to install an already installed mod, pax could do nothing and simply set explicit to true.
    • That way, you can:
      • ./pax add just enough resources with installs JER with "explicit": true & JEI with "explicit": false
      • ./pax add jei which sets JEI to "explicit": true
      • Removing JER would not remove JEI now.
  • Or we could add a new ./pax mark <name> --explicit command that would mark mods as explicit (The command is needed for later anyway in Mark mods as client/server only #22 (comment)).
    • That way, you can:
      • ./pax add just enough resources with installs JER with "explicit": true & JEI with "explicit": false
      • ./pax mark jei --explicit which sets JEI to "explicit": true
      • Removing JER would not remove JEI now.

badtz13 and others added 2 commits July 7, 2021 14:37
@Badtz13 Badtz13 requested a review from maradotwebp July 7, 2021 22:17
@Badtz13
Copy link
Contributor Author

Badtz13 commented Jul 7, 2021

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
Copy link
Owner

maradotwebp commented Jul 7, 2021

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

src/modpack/files.nim Show resolved Hide resolved
src/modpack/files.nim Outdated Show resolved Hide resolved
@maradotwebp maradotwebp changed the base branch from main to development July 7, 2021 22:59
@maradotwebp
Copy link
Owner

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
Copy link
Contributor

langedev commented Jul 7, 2021

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.

Co-authored-by: Fröhlich A. <[email protected]>
@maradotwebp maradotwebp merged commit d1928aa into maradotwebp:development Jul 7, 2021
@Badtz13 Badtz13 deleted the auto-depend branch July 7, 2021 23:05
@maradotwebp maradotwebp mentioned this pull request Jul 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fetch dependencies when installing & updating mods
3 participants