New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add cask reinstall command #1248

Merged
merged 6 commits into from Oct 24, 2016

Conversation

Projects
None yet
4 participants
@Git-Jiro
Copy link
Contributor

Git-Jiro commented Oct 9, 2016

This should implement: Homebrew/homebrew-cask#23998

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew tests with your changes locally?

end

# Always force uninstallation, ignore method parameter
Installer.new(cask, force: true).uninstall

This comment has been minimized.

@reitermarkus

reitermarkus Oct 9, 2016

Member

Uninstall should only be called if a cask is actually already installed, so add a check for cask.installed?.

skip_cask_deps: skip_cask_deps,
require_sha: require_sha).install
count += 1
rescue CaskAlreadyInstalledError => e

This comment has been minimized.

@reitermarkus

reitermarkus Oct 9, 2016

Member

I don't think we should rescue this, this would mean the uninstall step failed.

@reitermarkus

This comment has been minimized.

Copy link
Member

reitermarkus commented Oct 9, 2016

Just tested this locally, and it is failing because the cask file in the .metadata directory is removed when uninstalling, and install is trying to save this cask file to a newly created sub-directory of .metadata.

Also, would be great if you could also add some tests.

cc @Homebrew/cask

@Git-Jiro Git-Jiro force-pushed the Git-Jiro:add_reinstall_cmd_to_cask branch from 4584b94 to f756ba4 Oct 9, 2016

skip_cask_deps: skip_cask_deps,
require_sha: require_sha).install
count += 1
rescue CaskAlreadyInstalledAutoUpdatesError => e

This comment has been minimized.

@reitermarkus

reitermarkus Oct 9, 2016

Member

Also remove this, as this is basically the same error as CaskAlreadyInstalledError.

cask = Hbc.load(latest_installed_cask_file) if latest_installed_cask_file.exist?
end

if cask.installed?

This comment has been minimized.

@reitermarkus

reitermarkus Oct 9, 2016

Member

Everything above this, except cask = Hbc.load(cask_token) should also be inside this if statement.

it "allows reinstalling a Cask" do
Hbc::CLI::Install.run("local-transmission")
Hbc::CLI::Reinstall.run("local-transmission")
end

This comment has been minimized.

@reitermarkus

reitermarkus Oct 9, 2016

Member

This test should check if the files in local-transmission are actually installed.

They have to be installed after running Install, and they also have to be installed after running Reinstall.

There should be an equivalent test for the case where the cask is not already installed when running Reinstall.

@Git-Jiro Git-Jiro force-pushed the Git-Jiro:add_reinstall_cmd_to_cask branch 2 times, most recently from 294c30a to e769469 Oct 10, 2016

end

# reload cask to avoid 'No such file or directory' bug
cask = Hbc.load(cask_token)

This comment has been minimized.

@reitermarkus

reitermarkus Oct 10, 2016

Member

Do not re-load the cask here, instead change the variable used for uninstall.

@Git-Jiro Git-Jiro force-pushed the Git-Jiro:add_reinstall_cmd_to_cask branch from e769469 to 3223c20 Oct 11, 2016

@ilovezfs ilovezfs added the cask label Oct 11, 2016

@Git-Jiro

This comment has been minimized.

Copy link
Contributor

Git-Jiro commented Oct 12, 2016

@reitermarkus Hi, any ideas why the codecov stats are so low after my latest commit?

@reitermarkus
Copy link
Member

reitermarkus left a comment

Hi, any ideas why the codecov stats are so low after my latest commit?

No idea, sorry, maybe a rebase will help?


if cask.installed?
# use copy of cask for uninstallation to avoid 'No such file or directory' bug
uninstallCask = cask;

This comment has been minimized.

@reitermarkus

reitermarkus Oct 12, 2016

Member

Don't use camel case here. Also maybe rename it to installed_cask.

def self.help
"reinstalls the given Cask"
end

This comment has been minimized.

@reitermarkus

reitermarkus Oct 12, 2016

Member

Remove this empty line here.

Installer.new(cask,
force: force,
skip_cask_deps: skip_cask_deps,
require_sha: require_sha).install

This comment has been minimized.

@reitermarkus

reitermarkus Oct 12, 2016

Member

Align this parameter list with cask above.

module Hbc
class CLI
class Reinstall < Install

This comment has been minimized.

@reitermarkus

reitermarkus Oct 12, 2016

Member

Remove this empty line here.

require "test_helper"

describe Hbc::CLI::Reinstall do

This comment has been minimized.

@reitermarkus

reitermarkus Oct 12, 2016

Member

Remove this empty line here.

Hbc::CLI::Reinstall.run("local-transmission")
Hbc.load("local-transmission").must_be :installed?
end

This comment has been minimized.

@reitermarkus

reitermarkus Oct 12, 2016

Member

Remove this empty line here.

@@ -14,6 +14,7 @@
require "hbc/cli/home"
require "hbc/cli/info"
require "hbc/cli/install"
require "hbc/cli/reinstall"

This comment has been minimized.

@reitermarkus

reitermarkus Oct 12, 2016

Member

Could you sort this alphabetically?

@Git-Jiro Git-Jiro force-pushed the Git-Jiro:add_reinstall_cmd_to_cask branch from 3223c20 to a4e092a Oct 14, 2016

@Git-Jiro

This comment has been minimized.

Copy link
Contributor

Git-Jiro commented Oct 18, 2016

@reitermarkus I think I am seeing an issue with codecov: It looks like it stopped analyzing my latest pushes to my branch. Maybe because I squashed the commits and force pushed?

@reitermarkus

This comment has been minimized.

Copy link
Member

reitermarkus commented Oct 18, 2016

@Git-Jiro, I think I have found the reason the coverage is incorrect.

Try putting the run methods in the tests inside shutup blocks, like in the tests for Install.

@Git-Jiro Git-Jiro force-pushed the Git-Jiro:add_reinstall_cmd_to_cask branch from 8023bd3 to a519373 Oct 19, 2016

shutup do
Hbc::CLI::Uninstall.run("local-transmission")
end
Hbc.load("local-transmission").wont_be :installed?

This comment has been minimized.

@reitermarkus

reitermarkus Oct 19, 2016

Member

The indentation is off here.

shutup do
Hbc::CLI::Reinstall.run("local-transmission")
end

This comment has been minimized.

@reitermarkus

reitermarkus Oct 19, 2016

Member

Move this empty line before the shutup block, so the block is grouped with the Hbc.load line it belongs to. The same applies below.

@Git-Jiro Git-Jiro force-pushed the Git-Jiro:add_reinstall_cmd_to_cask branch from a519373 to 8f8606b Oct 20, 2016

@jawshooah
Copy link
Contributor

jawshooah left a comment

@reitermarkus something is wrong with our RuboCop setup, it should be catching this stuff in CI. Any idea why that's not happening?


if cask.installed?
# use copy of cask for uninstallation to avoid 'No such file or directory' bug
installed_cask = cask;

This comment has been minimized.

@jawshooah

jawshooah Oct 23, 2016

Contributor

The semicolon is unnecessary, please remove it.

@jawshooah
Copy link
Contributor

jawshooah left a comment

We need some documentation before we can merge. Please add an entry to brew-cask.1.md and run brew man to regenerate the man page, and commit both.

@reitermarkus

This comment has been minimized.

Copy link
Member

reitermarkus commented Oct 23, 2016

@reitermarkus something is wrong with our RuboCop setup, it should be catching this stuff in CI. Any idea why that's not happening?

This was because cask/**/* was excluded from brew style, but #1266 is merged now.

Git-Jiro added some commits Oct 23, 2016

Reinstall the given Cask.

<token> is usually the ID of a Cask as returned by `brew cask search`,
but see [OTHER WAYS TO SPECIFY A CASK][] for variations.

This comment has been minimized.

@reitermarkus

reitermarkus Oct 23, 2016

Member

I don't think we need to repeat what a token is here. Otherwise we would need to repeat if for every command that takes a token.

@jawshooah jawshooah merged commit 606a762 into Homebrew:master Oct 24, 2016

2 of 4 checks passed

codecov/patch 25.00% of diff hit (target 63.42%)
Details
codecov/project 60.76% (-2.67%) compared to 551ce2b
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
default Build finished.
Details
@jawshooah

This comment has been minimized.

Copy link
Contributor

jawshooah commented Oct 24, 2016

Thanks, @Git-Jiro!

@Git-Jiro Git-Jiro deleted the Git-Jiro:add_reinstall_cmd_to_cask branch Oct 24, 2016

kdeldycke added a commit to kdeldycke/meta-package-manager that referenced this pull request Nov 30, 2016

Fix update of cask packages.
Bump minimal requirement of cask to 1.1.0.

See: Homebrew/brew#1248

@Homebrew Homebrew locked and limited conversation to collaborators May 3, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.