version: add major, minor, patch methods #8183

SeekingMeaning · 2020-08-01T23:50:29Z

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 style with your changes locally?
Have you successfully run brew tests with your changes locally?

This adds five new methods to the Version class: major, minor, patch, major_minor, major_minor_patch. In particular, Version#major will be useful for determining the version suffix of gcc: Homebrew/homebrew-core#59485

(Based on cask/dsl/version)

Example usage

version_suffix = Formula["gcc"].linked_version.version.major

Examples in brew irb:

irb(main):001:0> Version.new("10.2")
=> #<Version:0x00007fa606210e90 @version="10.2">

irb(main):002:0> Version.new("10.2").major
=> #<Version::NumericToken 10>

irb(main):003:0> Version.new("10.2").minor
=> #<Version::NumericToken 2>

irb(main):004:0> Version.new("10.2").patch
=> nil

irb(main):005:0> Version.new("10.2").major_minor
=> #<Version:0x00007fb489879e30 @version="10.2">

irb(main):006:0> Version.new("10.2").major_minor_patch
=> #<Version:0x00007fb489885c08 @version="10.2">

SeekingMeaning · 2020-08-02T00:28:05Z

One thing I noticed is that Version::NumericTokens can't be compared with strings or integers:

irb(main):001:0> Version::NumericToken.new("1") == "1"
=> false
irb(main):002:0> Version::NumericToken.new("1") == 1
=> false

Whereas this works fine with Version:

irb(main):003:0> Version.new("1") == 1
=> true
irb(main):004:0> Version.new("1") == "1"
=> true

I might open another PR to address this

reitermarkus · 2020-08-02T14:25:20Z

Might be good to make this a module so it can be shared between both Cask::Version and Version.

MikeMcQuaid · 2020-08-03T07:55:40Z

In particular, Version#major will be useful for determining the version suffix of gcc, such as in Homebrew/homebrew-core#58973

Can you provide an example of how it should be used?

Might be good to make this a module so it can be shared between both Cask::Version and Version.

Agreed with this, I'd really love to see this deduplicate some Cask code.

dtrodrigues · 2020-08-10T12:57:15Z

Can a formula_suffix method be added to be used in audit.rb? The existing changes seem like they would allow the minor version parsing to be removed from audit.rb:

brew/Library/Homebrew/dev-cmd/audit.rb

Lines 754 to 760 in c49ef80

    
           stable_version_string = stable.version.to_s 
        
           stable_url_version = Version.parse(stable.url) 
        
           _, stable_url_minor_version, = stable_url_version.to_s 
        
                                                            .split(".", 3) 
        
                                                            .map(&:to_i) 
        
           formula_suffix = stable_version_string.split(".").last.to_i

gromgit · 2020-08-10T14:08:53Z

Can you provide an example of how it should be used?

It would turn the following incantation in the augustus formula:

with_env("HOMEBREW_CC" => Formula["gcc"].opt_bin/"gcc-#{Formula["gcc"].linked_version.to_s.slice(/\d+/)}") do

into:

with_env("HOMEBREW_CC" => Formula["gcc"].opt_bin/"gcc-#{Formula["gcc"].linked_version.version.major}") do

Not that much shorter, but somewhat more comprehensible to the Ruby-challenged like me.

There are at least 5 other formulae I'm tracking that could use the same improvement.

MikeMcQuaid · 2020-08-10T14:33:29Z

Not that much shorter, but somewhat more comprehensible to the Ruby-challenged like me.

Yup, makes sense. Can this be stripped down to the minimum number of methods we definitely have a use for in formulae and/or Homebrew/brew (including cask)?

SeekingMeaning · 2020-08-10T23:01:23Z

Can a formula_suffix method be added to be used in audit.rb? The existing changes seem like they would allow the minor version parsing to be removed from audit.rb:

brew/Library/Homebrew/dev-cmd/audit.rb

Lines 754 to 760 in c49ef80

stable_version_string = stable.version.to_s

stable_url_version = Version.parse(stable.url)

_, stable_url_minor_version, = stable_url_version.to_s

.split(".", 3)

.map(&:to_i)

formula_suffix = stable_version_string.split(".").last.to_i

Assuming stable_version_string.split(".").last.to_i always refers to the patch version (i.e. major.minor.patch), we could do something like this:

      stable_version_string = stable.version.to_s
      stable_url_version = Version.parse(stable.url)
      stable_url_minor_version = stable_url_version.minor.to_i

      formula_suffix = stable.version.patch.to_i

Also a few lines down we could replace stable_version_string.split(".")[0..1].join(".") with stable.version.major_minor.to_s

SeekingMeaning · 2020-08-10T23:15:58Z

Can this be stripped down to the minimum number of methods we definitely have a use for in formulae and/or Homebrew/brew (including cask)?

I think all of these methods except maybe minor_patch will have a use

major, minor, patch - used by the other methods
major specifically - can be useful for determining GCC prefix as Adrian mentioned above
major_minor - could be useful for includes such as -I#{include}/gupnp-1.2 where 1.2 can be replaced with Formula["gupnp"].version.major_minor

Might be good to make this a module so it can be shared between both Cask::Version and Version.

Agreed with this, I'd really love to see this deduplicate some Cask code.

I tried doing this but I don't think there's really any shared logic in how cask/dsl/version.rb and version.rb handle version tokens other than having the same method names (major_minor, etc.) Casks only use strings for versions and in general parse versions differently, while formulae use full-fledged object instances of the Version class. While we might be able to merge the two, we would have to do a lot of wrangling and be very careful to not introduce any regressions or unexpected behavior

MikeMcQuaid · 2020-08-11T08:06:36Z

I think all of these methods except maybe minor_patch will have a use

Let's skip this for now and otherwise can include them.

While we might be able to merge the two, we would have to do a lot of wrangling and be very careful to not introduce any regressions or unexpected behavior

This would be a great project to attempt! A general good start should be seeing if you can use our version logic without changing the tests.

dtrodrigues · 2020-08-11T12:42:49Z

Assuming stable_version_string.split(".").last.to_i always refers to the patch version (i.e. major.minor.patch), we could do something like this:

For some reason, I thought that it was almost always the patch version but not always. Looking through the existing throttled formulae though it does seem like it's always the patch, so just using patch would work and there isn't a need for an additional suffix method.

SeekingMeaning · 2020-08-11T21:07:26Z

Made a number of quality-of-life additions/changes, hopefully nothing will break

MikeMcQuaid

A few readability tweaks then 👍🏻 to 🚢 . Nice work @SeekingMeaning!

MikeMcQuaid · 2020-08-12T09:50:04Z

Library/Homebrew/version.rb

@@ -469,6 +486,26 @@ def <=>(other)
  end
  alias eql? ==

+  def major
+    tokens[0]


Suggested change

tokens[0]

tokens.first

MikeMcQuaid · 2020-08-12T09:50:13Z

Library/Homebrew/version.rb

+  end
+
+  def minor
+    tokens[1]


Suggested change

tokens[1]

tokens.second

MikeMcQuaid · 2020-08-12T09:50:18Z

Library/Homebrew/version.rb

+  end
+
+  def patch
+    tokens[2]


Suggested change

tokens[2]

tokens.third

MikeMcQuaid · 2020-08-12T09:50:33Z

Library/Homebrew/version.rb

+  end
+
+  def major_minor
+    Version.new([major, minor].reject(&:nil?).join("."))


Suggested change

Version.new([major, minor].reject(&:nil?).join("."))

Version.new([major, minor].compact.join("."))

MikeMcQuaid · 2020-08-12T09:50:42Z

Library/Homebrew/version.rb

+  end
+
+  def major_minor_patch
+    Version.new([major, minor, patch].reject(&:nil?).join("."))


Suggested change

Version.new([major, minor, patch].reject(&:nil?).join("."))

Version.new([major, minor, patch].compact.join("."))

SeekingMeaning added the in progress Maintainers are working on this label Aug 1, 2020

SeekingMeaning mentioned this pull request Aug 1, 2020

augustus: Get GCC version with public method Homebrew/homebrew-core#58973

Merged

5 tasks

SeekingMeaning marked this pull request as ready for review August 2, 2020 00:30

SeekingMeaning removed the in progress Maintainers are working on this label Aug 2, 2020

SeekingMeaning mentioned this pull request Aug 2, 2020

version: extend token comparisons to strings, integers, and nil #8186

Merged

6 tasks

SeekingMeaning force-pushed the version/tokens branch from 7b7a3fc to 1520675 Compare August 11, 2020 19:06

SeekingMeaning added 2 commits August 11, 2020 12:08

version: add major, minor, patch methods

0feb453

test/version_spec: add major/minor/patch examples

919e94b

SeekingMeaning force-pushed the version/tokens branch from 1520675 to 919e94b Compare August 11, 2020 19:10

SeekingMeaning added 7 commits August 11, 2020 12:22

version: add helper methods to Token class

f2112d6

version: allow versions to be compared against tokens

972b924

audit: use version token helper methods

e27f7b0

version/null: add major, minor, patch methods

266a944

pkg_version: delegate major, minor, patch methods to :version

51bc9ae

test/pkg_version_spec: add version token examples

e9b6a6d

pkg_version: disable rubocop hash alignment

f904cc2

SeekingMeaning mentioned this pull request Aug 11, 2020

Use version.major for GCC version suffix Homebrew/homebrew-core#59485

Merged

5 tasks

version: allow versions to be compared against NULL_TOKEN

fb96ccf

MikeMcQuaid approved these changes Aug 12, 2020

View reviewed changes

version: make major, minor, patch more idiomatic

1aceab0

SeekingMeaning merged commit 6d9bf72 into Homebrew:master Aug 12, 2020

SeekingMeaning deleted the version/tokens branch August 12, 2020 17:43

BrewTestBot added the outdated PR was locked due to age label Dec 18, 2020

Homebrew locked as resolved and limited conversation to collaborators Dec 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

version: add major, minor, patch methods #8183

version: add major, minor, patch methods #8183

SeekingMeaning commented Aug 1, 2020 •

edited

SeekingMeaning commented Aug 2, 2020 •

edited

reitermarkus commented Aug 2, 2020

MikeMcQuaid commented Aug 3, 2020

dtrodrigues commented Aug 10, 2020

gromgit commented Aug 10, 2020

MikeMcQuaid commented Aug 10, 2020

SeekingMeaning commented Aug 10, 2020

SeekingMeaning commented Aug 10, 2020 •

edited

MikeMcQuaid commented Aug 11, 2020

dtrodrigues commented Aug 11, 2020

SeekingMeaning commented Aug 11, 2020

MikeMcQuaid left a comment

MikeMcQuaid Aug 12, 2020

MikeMcQuaid Aug 12, 2020

MikeMcQuaid Aug 12, 2020

MikeMcQuaid Aug 12, 2020

MikeMcQuaid Aug 12, 2020

	Version.new([major, minor].reject(&:nil?).join("."))
	Version.new([major, minor].compact.join("."))

	Version.new([major, minor, patch].reject(&:nil?).join("."))
	Version.new([major, minor, patch].compact.join("."))

version: add major, minor, patch methods #8183

version: add major, minor, patch methods #8183

Conversation

SeekingMeaning commented Aug 1, 2020 • edited

Example usage

SeekingMeaning commented Aug 2, 2020 • edited

reitermarkus commented Aug 2, 2020

MikeMcQuaid commented Aug 3, 2020

dtrodrigues commented Aug 10, 2020

gromgit commented Aug 10, 2020

MikeMcQuaid commented Aug 10, 2020

SeekingMeaning commented Aug 10, 2020

SeekingMeaning commented Aug 10, 2020 • edited

MikeMcQuaid commented Aug 11, 2020

dtrodrigues commented Aug 11, 2020

SeekingMeaning commented Aug 11, 2020

MikeMcQuaid left a comment

Choose a reason for hiding this comment

MikeMcQuaid Aug 12, 2020

Choose a reason for hiding this comment

MikeMcQuaid Aug 12, 2020

Choose a reason for hiding this comment

MikeMcQuaid Aug 12, 2020

Choose a reason for hiding this comment

MikeMcQuaid Aug 12, 2020

Choose a reason for hiding this comment

MikeMcQuaid Aug 12, 2020

Choose a reason for hiding this comment

SeekingMeaning commented Aug 1, 2020 •

edited

SeekingMeaning commented Aug 2, 2020 •

edited

SeekingMeaning commented Aug 10, 2020 •

edited