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 command 'cask outdated' #2309

Merged
merged 1 commit into from Mar 16, 2017

Conversation

Projects
None yet
5 participants
@axiac
Copy link
Contributor

axiac commented Mar 10, 2017

  • 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?

Disclaimer for the unchecked item

A couple of weeks ago I was missing the brew cask outdated command. I checked the Issues and PRs on the Homebrew/brew repo and I couldn't find anything related so I decided to give it a try.
When I was ready to make a pull request I discovered PR #2250 was already created two days before and I learned about issue Homebrew/homebrew-cask#29301. I went back to coding, implemented the --greedy option too and now I have a complete PR for the cask outdated feature.

How it works:

When the --greedy options is not present in the command line, a Cask that has auto_updates true or version 'latest' is ignored, as required.

When --greedy is present, the value of auto_updates is ignored and the Cask is processed as explained below; the casks that have version 'latest' are always displayed as outdated.

The version of the Cask from tap is compared against the currently installed version of the application (versions.last). The Cask is outdated if the currently installed version is different than the version from the tap. In this case, all the local versions of the application are compared to the tap version and all that are different are reported as "outdated".

If the current installed version is the same as the tap version, the Cask is not reported as outdated, even if there are other (different) local versions.

Output formatting

The format of the command is the same as the output of brew outdated, including the processing of --verbose and --quiet command line options.

Other points of interest

The new functionality is accompanied by tests. The documentation (the man page) was updated.

@axiac axiac force-pushed the axiac:cask-outdated branch from 9b09f39 to b55a471 Mar 10, 2017

current = installed.last

# special case: tap version is 'latest' or current version is 'latest'
# changing from :latest to versioned or vice-versa probably never happens

This comment has been minimized.

@vitorgalvao

vitorgalvao Mar 11, 2017

Member

It does happen occasionally.

@vitorgalvao

This comment has been minimized.

Copy link
Member

vitorgalvao commented Mar 11, 2017

The Cask is outdated if the currently installed version is smaller than the version from the tap.

You can’t compare “size”, because versions are non-standard, for us. A version can just as well be a number or an intelligible string. The only comparison that can be made is “is it different?”.

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Mar 11, 2017

I think sortable versions might also be a prerequisite for this making any sense.

@axiac

This comment has been minimized.

Copy link
Contributor

axiac commented Mar 11, 2017

You both are right regarding the version comparison. I had semantic versioning in mind but I overlooked that, as strings, "10" is smaller than "9". And semantic versioning is a foolish assumption in this case, anyway.

Simplified the version comparison in a new commit.

class Outdated < Base
def self.run(*args)
greedy = args.include?("--greedy")
verbose = ($stdout.tty? || ARGV.verbose?) && !args.include?("--quiet")

This comment has been minimized.

@reitermarkus

reitermarkus Mar 11, 2017

Member

ARGV.verbose? -> verbose?

This comment has been minimized.

@axiac

axiac Mar 11, 2017

Contributor

Are you sure? It says: Error: undefined method `verbose?' for Hbc::CLI::Outdated:Class

This comment has been minimized.

@reitermarkus

reitermarkus Mar 11, 2017

Member

Ah, then it's CLI.verbose?, I thought it would use the CLI namespace.


# current installed version different than tap version
# collect all installed versions that are different than tap version and return them
installed.map { |v| v if v != version.to_s }.compact

This comment has been minimized.

@reitermarkus

reitermarkus Mar 11, 2017

Member

Use #select here.

This comment has been minimized.

@axiac

axiac Mar 11, 2017

Contributor

#select is the function I was looking for (but it seems I was too lazy to read the complete list of Array methods and I concluded too early). Thank you!

def outdated_versions(greedy = false)
greedy = true if greedy
@outdated ||= {}
@outdated[greedy] ||= _check_outdated(greedy)

This comment has been minimized.

@reitermarkus

reitermarkus Mar 11, 2017

Member

Let's not cache this. At the moment we are only using it only once anyways, but even if we were to use it multiple times, something will probably have changed between these calls.

This comment has been minimized.

@axiac

axiac Mar 11, 2017

Contributor

#outdated? and outdated_versions are both called when --verbose. This is why the information is cached (versions is expensive).

Is it ok to (drop caching and the #outdated_versions method and) let #outdated? return a boolean and the list of outdated versions?

outdated_versions = cask.outdated_versions(greedy)
outdated_info = "#{cask.token} (#{outdated_versions.join(", ")})"
current_version = cask.version.to_s
puts "#{outdated_info} < #{current_version}"

This comment has been minimized.

@reitermarkus

reitermarkus Mar 11, 2017

Member

Use != here instead of <.

This comment has been minimized.

@axiac

axiac Mar 11, 2017

Contributor

This is the output format used by brew outdated and it is also the one suggested in Homebrew/homebrew-cask#29301

This comment has been minimized.

@reitermarkus

reitermarkus Mar 11, 2017

Member

See the discussion here: #2250 (comment)

This comment has been minimized.

@axiac

axiac Mar 11, 2017

Contributor

I understood the reasons about using != to compare the versions and I changed the code.

Please remark the puts in front of that line. It is not a version comparison, "<" is part of the text that is displayed to the user:

$ brew cask outdated
cyberduck (5.3.7.23440, 5.3.8.23611) < 5.3.9.23624
virtualbox (5.1.14-112924) < 5.1.16-113841
virtualbox-extension-pack (5.1.14-112924) < 5.1.16-113841

This comment has been minimized.

@reitermarkus

reitermarkus Mar 11, 2017

Member

Yes, that is exactly the reason. Displaying < when we are only comparing with != doesn't make sense.

This comment has been minimized.

@axiac

axiac Mar 11, 2017

Contributor

I got your point now. But I still wonder what kind of magic allows brew outdated compare the Formula versions using <.

This comment has been minimized.

@reitermarkus

reitermarkus Mar 12, 2017

Member

This is because Formulae use the comparable PkgVersion class, and Hbc::DSL::Version is a subclass of String, so it only performs a String comparison.

@axiac axiac force-pushed the axiac:cask-outdated branch 4 times, most recently from 0814bd1 to 6049980 Mar 11, 2017

return installed if version.latest?

# not outdated unless there is a different version on tap
return [] unless current != version

This comment has been minimized.

@reitermarkus

reitermarkus Mar 12, 2017

Member

Make this if version == current.

return [] if version.nil?

# if --greedy is not present in the command line then skip when detection is not safe
unless greedy

This comment has been minimized.

@reitermarkus

reitermarkus Mar 12, 2017

Member

This should be:

      if greedy
        return versions if version.latest?
      else
        return [] if auto_updates
      end

Otherwise, when I have the :latest version installed, but there is an updated cask with version 1.0.0, it will not show up as outdated.

current = installed.last

# if version is "latest" then all installed version are outdated (greedy)
return installed if version.latest?

This comment has been minimized.

@reitermarkus

reitermarkus Mar 12, 2017

Member

This must be removed after changing the above to if greedy.

return [] if version.latest?
end

# keep the installed versions in a local variable because a call to :versions is expensive

This comment has been minimized.

@reitermarkus

reitermarkus Mar 12, 2017

Member

Remove this comment.

# not outdated unless there is a different version on tap
return [] unless current != version

# current installed version different than tap version

This comment has been minimized.

@reitermarkus

reitermarkus Mar 12, 2017

Member

Remove this comment.


# current installed version different than tap version
# collect all installed versions that are different than tap version and return them
installed.select { |v| v if v != version.to_s }

This comment has been minimized.

@reitermarkus

reitermarkus Mar 12, 2017

Member

No need for the if here, version != v is enough.

This comment has been minimized.

@reitermarkus

reitermarkus Mar 12, 2017

Member

Or even select(&version.method(:!=)).

@axiac axiac force-pushed the axiac:cask-outdated branch 2 times, most recently from 7994151 to 6df8290 Mar 12, 2017

return [] if current == version

# collect all installed versions that are different than tap version and return them
installed.select(&version.method(:!=))

This comment has been minimized.

@reitermarkus

reitermarkus Mar 12, 2017

Member

Ok, I changed my mind, I think select { |v| version != v } is clearer.

This comment has been minimized.

@axiac

axiac Mar 13, 2017

Contributor

It is clearer, indeed.

describe "outdated" do
it "ignores the Casks that have auto_updates true (without --greedy)" do
c = Hbc.load("auto-updates")
expect(c.outdated?).to be_falsey

This comment has been minimized.

@reitermarkus

reitermarkus Mar 12, 2017

Member

Make this expect(c).to be_outdated everywhere.

This comment has been minimized.

@axiac

axiac Mar 13, 2017

Contributor

It's expect(c).to_not be_outdated. Got it.

This comment has been minimized.

@reitermarkus

reitermarkus Mar 13, 2017

Member

Right, but use .not_to instead of .to_not.

cask = Hbc::CaskLoader.load_from_file(TEST_FIXTURE_DIR/"cask/Casks/outdated/local-transmission.rb")
InstallHelper.install_with_caskfile(cask)
end
Hbc::CLI.verbose = true

This comment has been minimized.

@reitermarkus

reitermarkus Mar 12, 2017

Member

Instead of this, use allow(Hbc::CLI).to receive(:verbose?).and_return(true).

This comment has been minimized.

@axiac

axiac Mar 13, 2017

Contributor

This eliminates the need to reset CLI.verbose in after, isn't it?

This comment has been minimized.

@reitermarkus
describe Hbc::CLI::Outdated, :cask do
before do
shutup do
cask = Hbc::CaskLoader.load_from_file(TEST_FIXTURE_DIR/"cask/Casks/basic-cask.rb")

This comment has been minimized.

@reitermarkus

reitermarkus Mar 12, 2017

Member

Define these like

let(:basic_cask) { Hbc::CaskLoader.load_from_file(TEST_FIXTURE_DIR/"cask/Casks/basic-cask.rb") }

above the before block.


after do
shutup do
Hbc::CLI::Uninstall.run("basic-cask", "local-caffeine", "local-transmission")

This comment has been minimized.

@reitermarkus

reitermarkus Mar 12, 2017

Member

I think these are automatically removed after each test.

@axiac axiac force-pushed the axiac:cask-outdated branch from 6df8290 to 82dc7e0 Mar 13, 2017

@joshka

This comment has been minimized.

Copy link
Contributor

joshka commented Mar 15, 2017

Heya, I'm disappointed that we ended up duplicating effort, but this looks much neater than my PR (#2250). I also learnt a bunch of ruby and rspec reading your version. Thanks :)

@joshka joshka referenced this pull request Mar 15, 2017

Closed

Add brew cask outdated command #2250

4 of 5 tasks complete
end
end

it "provides help" do

This comment has been minimized.

@reitermarkus

reitermarkus Mar 15, 2017

Member

No need to test this, since this is basically the same for every command and I think this is already tested elsewhere.

This comment has been minimized.

@axiac

axiac Mar 15, 2017

Contributor

@reitermarkus you want to spoil my perfect code coverage ;-)
I agree it is not needed; I just exercised the things you taught me in your previous review (.not_to, mocking method calls) and I enjoyed using them. I removed the test now and rebased the branch.

This comment has been minimized.

@reitermarkus

reitermarkus Mar 16, 2017

Member

@reitermarkus you want to spoil my perfect code coverage ;-)

Fixed that for you, by adding a test for the help methods. 😄

@axiac axiac force-pushed the axiac:cask-outdated branch from 82dc7e0 to 7532545 Mar 15, 2017

@axiac

This comment has been minimized.

Copy link
Contributor

axiac commented Mar 16, 2017

@joshka Thank you for your appreciation. I'm sorry my PR rendered your work useless, it was not my intention. You can still implement cask upgrade (the second part of issue Homebrew/homebrew-cask#29301), I promise I won't do it. I don't feel the need for it; cask reinstall does (almost) the same thing for outdated casks.

@reitermarkus reitermarkus merged commit c4d8b16 into Homebrew:master Mar 16, 2017

3 checks passed

codecov/patch 100% of diff hit (target 60.22%)
Details
codecov/project Absolute coverage decreased by -0.65% but relative coverage increased by +39.77% compared to 4d88cc4
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@reitermarkus

This comment has been minimized.

Copy link
Member

reitermarkus commented Mar 16, 2017

Thanks @axiac and @joshka for working on this!

@axiac axiac deleted the axiac:cask-outdated branch Mar 16, 2017

kdeldycke added a commit to kdeldycke/meta-package-manager that referenced this pull request Aug 31, 2017

@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.