Remove cask/cmd/zap
#14864
Remove cask/cmd/zap
#14864
Conversation
| it "can zap and unlink multiple Casks at once" do | ||
| caffeine = Cask::CaskLoader.load(cask_path("local-caffeine")) | ||
| transmission = Cask::CaskLoader.load(cask_path("local-transmission")) | ||
|
|
||
| Cask::Installer.new(caffeine).install | ||
| Cask::Installer.new(transmission).install | ||
|
|
||
| expect(caffeine).to be_installed | ||
| expect(transmission).to be_installed | ||
|
|
||
| described_class.run("local-caffeine", "local-transmission") | ||
|
|
||
| expect(caffeine).not_to be_installed | ||
| expect(caffeine.config.appdir.join("Caffeine.app")).not_to be_a_symlink | ||
| expect(transmission).not_to be_installed | ||
| expect(transmission.config.appdir.join("Transmission.app")).not_to be_a_symlink | ||
| end |
There was a problem hiding this comment.
I wonder if it would be possible to salvage some part of this test. We have a test for the zap artifact but not for the zap uninstall as a whole outside of this one. Maybe a one cask zap test could be added to cask/installer_spec.rb?
To be clear I don't think moving/adding that test should block this PR necessarily.
| casks.each do |cask| | ||
| odebug "Zapping Cask #{cask}" | ||
|
|
||
| raise Cask::CaskNotInstalledError, cask if !cask.installed? && !args.force? | ||
|
|
||
| Cask::Installer.new(cask, verbose: args.verbose?, force: args.force?).zap | ||
| end |
There was a problem hiding this comment.
I wonder if it'd be better to just have this in another file like Cask::Uninstall. Then, we could provide a similar interface to what we do with uninstalling kegs above and combine the zap and uninstall command logic a bit.
Cask::Uninstall.uninstall_casks(
*casks,
verbose: args.verbose?,
force: args.force?,
zap: args.zap?,
)
There was a problem hiding this comment.
@apainintheneck I dunno. I kinda like this with how minimal it is and avoiding the need for another file.
There was a problem hiding this comment.
In this case, it's just a few lines but once cask/cmd/uninstall gets removed a few more lines will be added. It's not the end of the word either way but I'd prefer to keep cask stuff in /cask and keep the commands leaner if possible. Another option would be to add them to the Uninstall module but that's not my first choice either.
There was a problem hiding this comment.
Let's just do this as-is now and require that change in cask/cmd/uninstall.
|
Thanks again @hyuraku! Looks good once CI is 🟢 |
Head branch was pushed to by a user without write access
hyuraku
force-pushed
the
remove-cask/cmd/zap
branch
from
7794bc3
to
4ab4dcd
Compare
brew stylewith your changes locally?brew typecheckwith your changes locally?brew testswith your changes locally?remove
cask/cmd/zapbecause of #14799