memoize version_select

[djbe]

Memoize the result of version_select, and allow us to change it from the Rakefile using a MIN_XCODE_VERSION constant

[AliSoftware]

The concept of the memoization is to still always call the method — which will either return immediately with the cached version, or compute it and cache it if it's the first time it's requested.

So don't change the call sites, otherwise you risk using an empty value if the value was never cached

[djbe]

👍

[AliSoftware]

Prefer the return "" if … construct, similar to a guard in Swift, to avoid having to indent the whole rest in a big else.

[AliSoftware]

NB: You could try to run rubocop on the ruby files to check for style (that's the ruby equivalent of swiftlint)

[djbe]

That gave me a
LocalJumpError: unexpected return

[AliSoftware]

Oh, right, you're in the begin block here

[djbe]

If I do

  def self.version_select
    @version_select ||= begin
      print "------ Checking xcode version ----"
      # if current Xcode already fulfills min version don't force DEVELOPER_DIR=…
      current_xcode = Pathname.new(`xcode-select -p`).parent.parent
      current_xcode_version = `mdls -name kMDItemVersion -raw #{current_xcode}`.chomp
      return "" if current_xcode_version.to_f >= MIN_XCODE_VERSION

The return interrupts the memoization and never sets the variable.

[AliSoftware]

Why not private anymore?

[djbe]

because it isn't a method.

[AliSoftware]

Yes, version_select is still a def'd method.

@@version_select is the property caching its result, but the method is still there. So we have to keep it private.

It's like if in Swift you had

func version_select() -> String {
  if let cached_vs = cached_vs { return cached_vs }
  … compute result
  cached_vs = result
}

version_select is still a method, using a property named cached_vs / @@version_select to store its cached value.

---

Idea: what we could do instead to make that code less complex is split that in two:

def self.compute_developer_dir
  # Same content as the version_select without memoizing, so basically what you have in that `begin…end` block
end

def self.version_select
  @@version_select ||= self.compute_developer_dir
end

Not sure what you prefer (but at least that would allow us an early return in self.compute_developer_dir)

[djbe]

FYI rubocop gives a massive number of warnings (115 to be exact, for 6 files).

[AliSoftware]

Yeah sadly not surprising, I'm not sure I ran it in a while

[AliSoftware]

If you don't use any of the parameters you can also completely remove them (do instead of do |task|)

[AliSoftware]

🤔 does that with with system ruby? I though that syntax was Ruby 2.1 or 2.2 only?

[djbe]

Afaik, it's ruby 1.9 syntax:

utils.rake:49:25: C: [Corrected] Use the new Ruby 1.9 hash syntax.
Octokit::Client.new(:access_token => token)
_____________________^^^^^^^^^^^^^^^

[AliSoftware]

Don't change this. Even if technically correct when considering this as a Ruby hash (and only in recent versions of Ruby), Rake used that syntax on purpose

Technically, Rake defines a def task(params) method internally to define rake tasks, and allows you to write:

• task :foo do to define a simple task without dependency. Which in practice is just calling task() with a single parameter :foo (of type symbol), defining the name of the task
• task :code => :install do to define a task with a single dependency. The use of => is expected to give the visual meaning of dependency. In practice what you do is pass a single parameter to that task method which happens to be a Hash (dictionary) with a single key (:code) and a value (:install)… but that's implementation detail
• task :code => [:x, :y, :z] do to define multiple dependencies, once again that => arrow is supposed to represent the visual cue of dependency, even if in practice what you do is pass a single parameter of type Hash with a single key :code, whose value is an array of 3 values… but again that's an implementation detail

So in practice Rake chose to use that implementation detail because that => was a nice visual cue for dependency (and not because it was technically a hash, which I actually never realised it was in that case until recently). So I think for the special case of task declarations, it's better to keep the => syntax, whatever rubocop says.

[djbe]

👍