ReVanced/revanced-cli

feat: Index patches uniquely

Ushie opened this issue · 11 comments

Ushie commented

Feature description

Add index numbers to patches, can be used everywhere where a patch name is an argument

Motivation

Currently, there's absolutely nothing preventing multiple patch names from clashing, if someone adds a "Hide ads" universal patch then -i "Hide ads" would include both the app-specific and the universal patch, and there's no way to pick one and not the other.

Acknowledgements

  • This request is not a duplicate of an existing issue.
  • I have chosen an appropriate title.
  • All requested information has been provided properly.

How would you design this? What option name would you use? Where would the indices be listed?

Ushie commented

Option name as what? to display the index? they'd just be shown in list-patches as is the same place you'd get the patch name to be able to include it

I'm unsure what the list-patches output looks like, as I haven't used CLI in a while but the ideal placement should be obvious right away once someone looks at the output

How would you included or exclude patches by index?

Ushie commented

As including by patch name REQUIRES quotes, simply -i 1 would work fine as it can't possibly clash with patch names

Quotes are part of the parser, you do not need quotes to reference a patch as you can type -i patchname.
If there is a patch called 1 it would interfere with the index, which is why a different approach is necessary.

Ushie commented

Well, that changes things, what about --include-index 1,2,3,4,5? I know that with the current CLI usage, it would be more like --include-index 1 --include-index 2 and so on, but I feel like that would be quite annoying, I'm not sure what we'd name the short-flag either as -ii is weird (but I'm fine with)

If --include-index 1,2,3,4,5 is possible, then we can also expand that to other flags like --include and --exclude

but I feel like that would be quite annoying

The way it is right now is the correct syntax and a global standard, also used by PicoCLI. Each array element must be inputted using the option name -i a -i b. Parsing -i a,b breaks the standard and requires us to parse the singular string input a,b and its variations such as "a",b, a, b, a, "b" or "a,b",c and so on which is not a viable solution to begin with.

If --include-index 1,2,3,4,5 is possible

The syntax would look like this for example:

-i name --ii 2 --include-index 3 -i name2 -e name3 -ei 4

Alternatively it may be possible to use subcommand such as:

-i --index 1 -i name

Ushie commented

The thing is, this is expected to be the "main" way of selecting patches

I feel like the need to add --index would complicate it further, I'm not sure, what about the invert?

The thing is, this is expected to be the "main" way of selecting patches

There is no "main" way. You can either select by name or by index. Index works deterministically for the current input of patches, whereas name always works with the unavoidable side effect of clashes. Both have their use cases; index would solve the clashing problem, whereas names would solve the problem of the index changing every time. Using both has to be judged accordingly to every situation individually.

I feel like the need to add --index would complicate it further, I'm not sure, what about the invert?

Yes, it adds a new, nearly identical option. What do you mean by invert?

Ushie commented

There is no "main" way. You can either select by name or by index.

Wouldn't that statement require it to also be -i --name? otherwise name would be considered the main way of selecting patches

What do you mean by invert?

I was suggesting -i 1 -i --name patchname, as index is more reliable to include what you actually want to include, you might not know that another patch would conflict with it and naturally just do it the "normal" way which is with the name

Wouldn't that statement require it to also be -i --name?

You can either do -i patchname or -ii 1 in case the patch has the index 1