maradotwebp / pax

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

Mark command #42

Open Badtz13 opened 3 years ago

Badtz13 commented 3 years ago

Implements a mark command that allows the user to tag mods as serverside, clientside or both, with both being the default assigned value. Additionally, the command allow for manually toggling mod's 'explicit' status. For example, if you install Just Enough Resources, JEI will be installed as a dependency implicitly. However, JEI is probably a mod that the user wants to be in the pack even if they later decide to remove JER, so they can manually tag JEI as explicit.

Usage: pax mark <modname> <markname>

Possible markname values:

The output really needs some work, but I am unsure how to extend the current echo paradigm.

Should close #41

Badtz13 commented 3 years ago

I am also not sure if you want us to be requesting to merge into the development branch or the main branch. If you want pulls to be pointed towards the development branch, please keep it up to date.

maradotwebp commented 3 years ago

Development branch please. Should normally be up-to-date.

Badtz13 commented 3 years ago

Implemented version pinning in the manifest, format looks like this:

    {
      "projectID": 243121,
      "fileID": 3366626,
      "required": true,
      "__meta": {
        "name": "Quark",
        "explicit": false,
        "installOn": "both",
        "pinned": true,
        "dependencies": [
          250363
        ]
      }
    },
maradotwebp commented 3 years ago

Ok, but I would recommend a seperate pull request for pinning - it's not a big feature, but it's better to keep it seperate.

Also, for marking, I'd rather have it implemented as ./pax mark <modname> with -c, --client-only, -s, --server-only, -e, --explicit, -i, --implicit options instead of your method for two reasons:

Badtz13 commented 3 years ago

I can implement it like that if you want, but it seemed a bit excessive to have individual flags for each type. Essentially we would need:

And maybe eventually more? We already have discussed a --pinned / --unpinned flag, and I can see a use case where users might want to custom-mark certain mods as something else. Imagine something like a lite version of a modpack, where the pack creator might want to only export the baseline required mods. A custom marking would be nice no? If all of these custom flags are the way you want it implemented, I will go through with it, but I think that you are opening yourself up to a lot of pain in the future.

To address the points you made: Maybe this is my lack of knowledge on Therapist speaking, but I assume there should be a way to allow for multiple markings in the current implementation? I think that ./pax mark jei pinned client is not super bad. Also I don't know how pax usually works for you, but I always have to put quotes around modnames that have spaces currently. (I am running fish on arch) so I don't really see that as a major issue.

Another third option, which might not be possible (Again, I don't yet know Therapist very well), but would be nice would be some sort of global flags that would work with other commands as well. It would be really cool if you could simply mark mods when installing them: ./pax add jei client both

maradotwebp commented 3 years ago

We already have discussed a --pinned / --unpinned flag, and I can see a use case where users might want to custom-mark certain mods as something else. Imagine something like a lite version of a modpack, where the pack creator might want to only export the baseline required mods.

I've got a friend who distributes two diffferent versions of a Fabric modpack (one with Sodium included, one with Iris) like this in his CI:

# Sodium is included in the default modpack
./pax export # The version with Sodium
./pax remove sodium -y
./pax add 455508 -y
./pax export # The version with Iris

This is already possible, I don't think implementing a custom-mark command would be beneficial on top of that - duplicate functionality can be confusing.

To address the points you made: Maybe this is my lack of knowledge on Therapist speaking, but I assume there should be a way to allow for multiple markings in the current implementation?

You can set multi=true in an argument (like it already is set in on the addCmd) which would collect arbitrarily many arguments into one (./pax mark abc def ghi jkl would be "abc def ghi jkl") which you could then split on the space character.

Also I don't know how pax usually works for you, but I always have to put quotes around modnames that have spaces currently.

./pax add just enough resources works due to the multi=true mentioned above, I simply haven't gotten around to adding it to the other commands ^^

Another third option, which might not be possible (Again, I don't yet know Therapist very well), but would be nice would be some sort of global flags that would work with other commands as well. It would be really cool if you could simply mark mods when installing them: ./pax add jei client both

Afaik that's not possible, but you can construct a therapist argument new<Type>Arg outside of an cmd and then simply use the variable multiple times in whatever commands you want, including the root spec command.


In my opinion, it would be best to go ahead with the specifying marking as an optional parameter (--client-only, --server-only, etc.). Since I plan to implement multi=true on other commands and you plan implementing these parameters on the add command too (nice idea btw), it would be best to specify the marking as options (./pax add just enough resources --server-only -- explicit) instead of arguments (./pax add just enough resources server-only explicit, which is much less readable and afaik not even possible to parse)


Some more points:

langedev commented 3 years ago

I think the flags are a good idea to allow pax add and pax mark to share functionality. That said I think having a whole list of flags is pretty ugly, and difficult to use. Generally in CLI development when you have a multifunctionality option like this you set a flag with a value after it something like "--process=server" or "--process=client" or "--process=both." (this also allows us to still use 'both' which I think is clear in this context, but not helpful when used like "--both.")

Therapist can do this sort of functionality like so

process: newStringArg(@["-p", "--process"], defaultVal="both", 
                      help="Which process (client/server/both) to run on")

This seems to fit all the use cases you were both discussing with one expection, which is it doesn't allow custom marks in an intuitive way like the original pull request. Something like pax mark "quark" light to add a custom "light" mark is probably best implemented with a comma seperated list: pax mark quark --custom="light,someOtherTag,vazkii" I would argue that this method of marking with custom tags is NOT substituted by

# Sodium is included in the default modpack
./pax export # The version with Sodium
./pax remove sodium -y
./pax add 455508 -y
./pax export # The version with Iris

It fails when you need to add or remove many mods for your different versions. An example of this would be "Life in the Village" and "Live in the Village Lite" which differ by 10 mods. While I think this is a niche use case, I would argue customtags are easy to implement and solve this problem quite well.

maradotwebp commented 3 years ago

I think the flags are a good idea to allow pax add and pax mark to share functionality. That said I think having a whole list of flags is pretty ugly, and difficult to use. Generally in CLI development when you have a multifunctionality option like this you set a flag with a value after it something like "--process=server" or "--process=client" or "--process=both." (this also allows us to still use 'both' which I think is clear in this context, but not helpful when used like "--both.")

Agreed. specifying --process=server (or --side=server, which I'd prefer) is superior to --server-only. We can do it that way.

This seems to fit all the use cases you were both discussing with one expection, which is it doesn't allow custom marks in an intuitive way like the original pull request. Something like pax mark "quark" light to add a custom "light" mark is probably best implemented with a comma seperated list: pax mark quark --custom="light,someOtherTag,vazkii". I would argue that this method of marking with custom tags is NOT substituted by just adding/removing mods in a script.

But there's still a problem with custom tags: How do you remove custom tags, if you specify them with ./pax mark quark --custom="light"?

The simple adding/removing scripts still seem a superior alternative to custom tags to me, even for adding/removing lots of mods (you can just write a shell script or put it in the ci directly).

Can you think of a problem custom tags would solve that wouldn't be possible with just the existing commands? I'd be more inclined to let you implement them if such an issue came up... ;)