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

Mark command #42

Draft
wants to merge 16 commits into
base: development
Choose a base branch
from
Draft

Mark command #42

wants to merge 16 commits into from

Conversation

Badtz13
Copy link
Contributor

@Badtz13 Badtz13 commented Jul 8, 2021

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:

  • implicit
  • explicit
  • server
  • client
  • both

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

Should close #41

@Badtz13 Badtz13 changed the base branch from main to development July 8, 2021 07:06
@Badtz13 Badtz13 changed the base branch from development to main July 8, 2021 07:07
@Badtz13
Copy link
Contributor Author

Badtz13 commented Jul 8, 2021

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

maradotwebp commented Jul 8, 2021

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

@Badtz13 Badtz13 changed the base branch from main to development July 8, 2021 17:16
Co-authored-by: PanDoes <public@daltonlange.com>
@Badtz13
Copy link
Contributor Author

Badtz13 commented Jul 8, 2021

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

maradotwebp commented Jul 8, 2021

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:

  • You can mark mods with multiple options in one command.
  • You can mark mods with spaces in their name, like ./pax mark just enough resources --client-only --explicit

@Badtz13 Badtz13 marked this pull request as draft July 8, 2021 18:03
@Badtz13
Copy link
Contributor Author

Badtz13 commented Jul 10, 2021

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:

  • --client-only
  • --server-only
  • --both
  • --implicit
  • --explicit

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

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:

  • The names of the parameters --server-only and --client-only sound good to me, but the name of --both is a bit confusing. Maybe something like --client-server or some other name that implies that the mod works on both sides?

@langedev
Copy link
Contributor

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

maradotwebp commented Jul 15, 2021

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... ;)

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.

Implement mark command
3 participants