#326: Improved UninstallCommandlet to uninstall multiple tools #334
Conversation
Pull Request Test Coverage Report for Build 9173412922Details
💛 - Coveralls |
There was a problem hiding this comment.
@ndemirca thanks for your PR and the nice idea to support uninstalling multiple tools in one call. 👍
Please have a look at my review comments and see what you think.
IMHO the current approach is not really consistent. Most probably we have to rethink how to implement this properly:
- Would we create a
ToolListProperty? - Or would it be even smarter to change the general design of
Propertyto allow some cardinality so every property can take any number of elements and by default the maximum is 1 and the minimum is 0 if not required and 1 if required? The cardinality could then be specified as an additional constructor argument to allow multiple parameters makingStringListPropertypointless and removed (instead usingStringPropertywith any number of parameters).
|
|
||
| /** The tool to uninstall. */ | ||
| public final ToolProperty tool; | ||
| public class UninstallCommandlet extends LocalToolCommandlet { |
There was a problem hiding this comment.
Inheriting from LocalToolCommandlet says that Uninstall is a tool that can be installed.
You can now do ide install uninstall and it will try to find a tool uninstall in ide-urls.
Also auto-completion will now suggest uninstall.
IMHO it is wrong to extend from LocalToolCommandlet here.
| @@ -90,6 +90,11 @@ | |||
| <artifactId>jansi</artifactId> | |||
| <version>${jansi.version}</version> | |||
| </dependency> | |||
| <dependency> | |||
| <groupId>net.bytebuddy</groupId> | |||
| <artifactId>byte-buddy</artifactId> | |||
There was a problem hiding this comment.
why do we need byte-buddy now and what does it have to do with this story?
| @@ -318,6 +318,9 @@ public final class Tag { | |||
| /** {@link #Tag} for encryption. */ | |||
| public static final Tag ENCRYPTION = create("encryption", CRYPTO); | |||
|
|
|||
| /** {@link #Tag} for uninstall. */ | |||
| public static final Tag UNINSTALL = create("uninstall", ROOT, false, "remove"); | |||
There was a problem hiding this comment.
I also would question this tag. The idea of tags is to choose tools and plugins.
What else could/would be tagged with uninstall? Tagging xyz with xyz does not really add much value unless xyz is a category that groups multiple things beyond xyz.
Closes: #326