OS::Mac: Move version methods into ::Version

[samford]

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

---

#11557 needs to know if a Sparkle feed item's minimumSystemVersion (a macOS version like 12.0.0) is a prerelease version. OS::Mac has a #prerelease? method but it can seemingly only be used to check the macOS version of the execution environment, like OS::Mac.prerelease?. We need to be able to do OS::Mac::Version.new("12.0.0").prerelease?, which isn't currently supported.

This PR moves the #outdated_release? and #prerelease? methods to OS::Mac::Version and simply calls the ::Version methods in the related OS::Mac methods. This allows for both OS::Mac.prerelease? and OS::Mac::Version.new("12.0.0").prerelease?.

It would be nice to be able to remove these methods from OS::Mac while still being able to access them like OS::Mac.prerelease?. I couldn't get either def_delegators or delegate (from ActiveSupport) to work where OS::Mac.prerelease? calls version.prerelease?). Any suggestions on how to approach this or is the present code what we would expect?

[BrewTestBot]

Review period will end on 2021-06-23 at 16:18:45 UTC.

[donnacn]

I'm new to this, might need some help with the technical side with fixing issues....

[maxim-belkin]

👍🏻 from me. Moving hard-coded versions to os/mac/version seems like a logical step to me because os/mac behaves as an aggregator of macOS-specific behavior. I specifically like that this PR defines two new constants that we can quickly update or, if desired, read or obtain from somewhere later down the road.

[maxim-belkin]

Tested with:

irb(main):013:0> v = OS::Mac::Version.new("10")
=> #<OS::Mac::Version:0x00007fa4bc070310 @version="10", @detected_from_url=false, @comparison_cache={}>
irb(main):014:0> v.prerelease?
=> false
irb(main):015:0> v.outdated_release?
=> true
irb(main):016:0> 

[samford]

I modified the ::Version methods to use self instead of the version variable, as it wasn't working correctly otherwise (i.e., #prerelease? was returning false for a Version like OS::Mac::Version.new("12")). I had previously modified these to use self but I lost that change while iterating on different versions of this.

---

Tested with:

For the same of completeness...

Here's the output for a prerelease version:

irb(main):001:0> v = OS::Mac::Version.new("12")
=> #<OS::Mac::Version:0x00007f86499ace60 @version="12", @detected_from_url=false, @comparison_cache={}>
irb(main):002:0> v.outdated_release?
=> false
irb(main):003:0> v.prerelease?
=> true

Also, the existing behavior of OS::Mac.prerelease? and OS::Mac.outdated_release? remains the same:

irb(main):001:0> OS::Mac.version
=> #<OS::Mac::Version:0x00007f9c4aa31ec8 @version="11", @detected_from_url=false, @comparison_cache={}>
irb(main):002:0> OS::Mac.outdated_release?
=> false
irb(main):003:0> OS::Mac.prerelease?
=> false

[MikeMcQuaid]

Makes sense to me, nice API addition/cleanup. One optional thought but feel free to merge without if you'd rather.

[samford]

I was pondering the OS::Mac#outdated_release?/OS::Mac#prerelease? situation and what do we think about updating existing references in brew to simply use OS::Mac.version.outdated_release?/OS::Mac.version.prerelease?. The methods in OS::Mac are only using version to call the methods in ::Version, so they're just a convenience at this point.

This would allow us to fully remove the methods from OS::Mac and it would also make the code more clear. To me OS::Mac.prerelease? doesn't immediately make me think that it's checking the macOS version of the execution environment but OS::Mac.version.prerelease? is more explicit and gives me a better idea of what may be happening. Maybe this is just me, though.

[MikeMcQuaid]

I was pondering the OS::Mac#outdated_release?/OS::Mac#prerelease? situation and what do we think about updating existing references in brew to simply use OS::Mac.version.outdated_release?/OS::Mac.version.prerelease?. The methods in OS::Mac are only using version to call the methods in ::Version, so they're just a convenience at this point.

This would allow us to fully remove the methods from OS::Mac and it would also make the code more clear. To me OS::Mac.prerelease? doesn't immediately make me think that it's checking the macOS version of the execution environment but OS::Mac.version.prerelease? is more explicit and gives me a better idea of what may be happening. Maybe this is just me, though.

Makes sense to me 👍🏻

[samford]

In the latest changes, the new constants in OS::Mac::Version are private and I've updated usage of the aforementioned OS::Mac methods and properly removed them. Everything in brew now uses OS::Mac.version to access these methods.

Unless anyone sees anything else that needs to be addressed, I think that should take care of this.

[BrewTestBot]

Review period ended.

[samford]

Thanks, everyone!