Skip to content

add command 'cask outdated'#2309

Merged
reitermarkus merged 1 commit intoHomebrew:masterfrom
axiac:cask-outdated
Mar 16, 2017
Merged

add command 'cask outdated'#2309
reitermarkus merged 1 commit intoHomebrew:masterfrom
axiac:cask-outdated

Conversation

@axiac
Copy link
Copy Markdown
Contributor

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

Comment thread Library/Homebrew/cask/lib/hbc/cask.rb Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does happen occasionally.

@vitorgalvao
Copy link
Copy Markdown
Contributor

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
Copy link
Copy Markdown
Contributor

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

@axiac
Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ARGV.verbose? -> verbose?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread Library/Homebrew/cask/lib/hbc/cask.rb Outdated
Copy link
Copy Markdown
Member

@reitermarkus reitermarkus Mar 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use #select here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#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!

Comment thread Library/Homebrew/cask/lib/hbc/cask.rb Outdated
Copy link
Copy Markdown
Member

@reitermarkus reitermarkus Mar 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use != here instead of <.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the discussion here: #2250 (comment)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 cask-outdated branch 4 times, most recently from 0814bd1 to 6049980 Compare March 11, 2017 23:36
Comment thread Library/Homebrew/cask/lib/hbc/cask.rb Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this if version == current.

Comment thread Library/Homebrew/cask/lib/hbc/cask.rb Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread Library/Homebrew/cask/lib/hbc/cask.rb Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread Library/Homebrew/cask/lib/hbc/cask.rb Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this comment.

Comment thread Library/Homebrew/cask/lib/hbc/cask.rb Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this comment.

Comment thread Library/Homebrew/cask/lib/hbc/cask.rb Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@axiac axiac force-pushed the cask-outdated branch 2 times, most recently from 7994151 to 6df8290 Compare March 12, 2017 19:10
Comment thread Library/Homebrew/cask/lib/hbc/cask.rb Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is clearer, indeed.

Comment thread Library/Homebrew/test/cask/cask_spec.rb Outdated
Copy link
Copy Markdown
Member

@reitermarkus reitermarkus Mar 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this expect(c).to be_outdated everywhere.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

@reitermarkus reitermarkus Mar 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but use .not_to instead of .to_not.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly.

Copy link
Copy Markdown
Member

@reitermarkus reitermarkus Mar 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Define these like

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

above the before block.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these are automatically removed after each test.

@joshka
Copy link
Copy Markdown
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 mentioned this pull request Mar 15, 2017
5 tasks
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@axiac
Copy link
Copy Markdown
Contributor Author

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
@reitermarkus
Copy link
Copy Markdown
Member

Thanks @axiac and @joshka for working on this!

@axiac axiac deleted the cask-outdated branch March 16, 2017 18:59
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
@axiac axiac restored the cask-outdated branch March 13, 2021 11:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants