Skip to content

Commit

Permalink
fix: Make clear what the --exclusive command actually does
Browse files Browse the repository at this point in the history
  • Loading branch information
TheJeterLP authored Jul 10, 2022
1 parent ab4c819 commit a26b0ea
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion src/main/kotlin/app/revanced/cli/command/MainCommand.kt
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ internal object MainCommand : Runnable {
@Option(names = ["-e", "--exclude"], description = ["Explicitly exclude patches"])

This comment has been minimized.

Copy link
@oSumAtrIX

oSumAtrIX Jul 10, 2022

Member

@TheJeterLP You forgot this. Something like "Exclude patches" suffices.

This comment has been minimized.

Copy link
@TheJeterLP

TheJeterLP Jul 10, 2022

Author Contributor

Nope, that description is indeed clear for everyone. No one ever asked what that does

This comment has been minimized.

Copy link
@oSumAtrIX

oSumAtrIX Jul 10, 2022

Member

Exclusively include patches and Explicitly exclude patches are equally clear. Either change or leave both.

This comment has been minimized.

Copy link
@TheJeterLP

TheJeterLP Jul 10, 2022

Author Contributor

If no one asks about what something does, that means that the general user understands a description. If they have to ask what something does, it means they don't understand the description.

This comment has been minimized.

Copy link
@oSumAtrIX

oSumAtrIX Jul 10, 2022

Member

No this is logically flawed. Necessity and sufficiency are different things. No one asking does not suffice. Exclusively include patches and Explicitly exclude patches are equally clear.

This comment has been minimized.

Copy link
@Sculas

Sculas Jul 10, 2022

Contributor

Patches to exclude would be better for this one. As for the other one, Exclusively include patches is unclear. So that change is good.

This comment has been minimized.

Copy link
@oSumAtrIX

oSumAtrIX Jul 10, 2022

Member

Yep, which is why I recommended changing this one as well.

This comment has been minimized.

Copy link
@oSumAtrIX

oSumAtrIX Jul 10, 2022

Member

Also since @TheJeterLP uses the active tense Only installs... it should say Excludes patches.

This comment has been minimized.

Copy link
@Sculas

Sculas Jul 10, 2022

Contributor

Consistency is a good thing. But it's not like the world will end if this doesn't get changed. Since it currently suffices, it's a low priority and can be changed in the future if still needed.

This comment has been minimized.

Copy link
@Sculas

Sculas Jul 10, 2022

Contributor

Also since @TheJeterLP uses the active tense Only installs... it should say Excludes patches.

Excludes patches sounds wrong. It's a help message after all. You may understand what it means while some others may not. Keep that in mind.

This comment has been minimized.

Copy link
@oSumAtrIX

oSumAtrIX Jul 10, 2022

Member

In that case, it is important to change Only installs the patches ... for the --exclusive option too as it is in the same active tense.

This comment has been minimized.

Copy link
@Sculas

Sculas Jul 10, 2022

Contributor

@oSumAtrIX Feel free to make an issue for this change. Keep in mind, that it's a low priority and doesn't need to be changed right now.

This comment has been minimized.

Copy link
@Sculas

Sculas Jul 10, 2022

Contributor

We might as well just redo all help messages, so they're consistent. Since now multiple contributors have written help messages, making them inconsistent. What do you think?

This comment has been minimized.

Copy link
@oSumAtrIX

oSumAtrIX Jul 10, 2022

Member

Already creating an issue haha:
image

var excludedPatches = arrayOf<String>()

@Option(names = ["--exclusive"], description = ["Exclusively include patches"])
@Option(names = ["--exclusive"], description = ["Only installs the patches you include, not including any patch by default"])
var defaultExclude = false

@Option(names = ["-i", "--include"], description = ["Include patches"])
Expand Down

2 comments on commit a26b0ea

@l4v3nx
Copy link

@l4v3nx l4v3nx commented on a26b0ea Jul 10, 2022

Choose a reason for hiding this comment

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

Only installs the patches you include, not including any patch by default

what are the patches that includes by default?

@TheJeterLP
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only installs the patches you include, not including any patch by default

what are the patches that includes by default?

The ones that get installed by default. See CLI output for the default patches.

Please sign in to comment.