Include tooling removal methods #69
Conversation
* Added `RokitManifest::remove_tool` to remove a tool from the manifests * Added `ToolStorage::remove_tool_link` to remove a link for an installed tool
CompeyDev
changed the title
There was a problem hiding this comment.
Went through the code and it looks good overall, and it looks like this is mostly copied from the add command, so I just left a few nits.
However, the reason the remove command was not implemented before Rokit's release is because there were some additional things to consider. They all stem from the fact that Rokit does not and can not know about all rokit.toml files on the system.
- Should the
removecommand only remove tools from the single manifest, similar toadd? If so, how do we communicate to a user that the tool may still exist on their system, just not in the chosen manifest? - If the command should remove tool binaries, which ones does it remove?
- All of them, requiring a reinstall in Rokit projects that may still be using the tool
- Only the one listed in the manifest, leaving remaining binaries, but the exact tool may still need a reinstall in other Rokit projects
- If the command should remove tool links, how do we make sure this is actually correct? A user can have a tool such as
rojo = "Kampfkarren/[email protected]"if they so desire, and this would actually need therojolink to still exist.
I dont have a good answer to these questions but it seems to me like #70 considered it to some extent (it removed the link only when all tool binaries were gone).
Maybe it would be good to have some combination of a remove + uninstall subcommand, where uninstall nukes the binaries while remove only deals with manifests.
| @@ -1,5 +1,6 @@ | |||
| use anyhow::{Context, Result}; | |||
| use clap::{ArgAction, CommandFactory, Parser}; | |||
| use remove::RemoveSubcommand; | |||
There was a problem hiding this comment.
I assume this was an auto import, so rust-analyzer put it up here, but all the imports for commands are grouped below so the new import should be there as well.
Co-authored-by: Filip Tibell <[email protected]>
Co-authored-by: Filip Tibell <[email protected]>
Co-authored-by: Filip Tibell <[email protected]>
Co-authored-by: Filip Tibell <[email protected]>
Co-authored-by: Filip Tibell <[email protected]>
|
@filiptibell any plans on reviewing this PR again? |
I haven't yet addressed all the comments on this PR, and I do not think I will be able to anytime soon. The feature is pretty simple to implement, and I have enabled maintainer commits, so @filiptibell should be able to finalize and merge it. |
3 participants
Adds a
rokit removecommand to remove installed tools, which works similar torokit add.Closes #3.