release-toolkit icon indicating copy to clipboard operation
release-toolkit copied to clipboard

Refactor *VersionHelpers to use an object

Open AliSoftware opened this issue 5 years ago • 2 comments

Today ios_version_helper and android_version_helper consist of freeform, module-static methods which take various and heterogeneous types of parameters depending on the method (sometimes a String for versionName, sometimes a Hash, etc)

It would make way more sense, and make the code way easier to understand, if instead of freeform methods, we created a class Version which would expose the appropriate instance properties (e.g. for Android, name and code) and instance methods to manipulate those versions, inspired by Gem::Version and its methods.

This would make the implementation way easier to understand and document (not having to repeat the parameter type documentation methods taking similar inputs), but also clearer at call-site and more idiomatic.


Some ideas / inspiration for possible API, e.g. for the Android one in the Fastlane::Helper::Android module:

class Version
  # [String] Doc here
  attr_reader :name
  # [Int] Doc here
  attr_reader :code

  # @return [Version] Next release version
  def next_release
    …
  end
  …
end

Or, alternatively, something like:

class Version
  attr_reader :major, :minor, :hotfix
  attr_reader :is_alpha?
  attr_reader :suffix
  attr_reader :code

  # @return [String] The versionName. `"alpha-major.minor.patch-suffix"` if `is_alpha?`, `"major.minor.patch-rc-suffix"` otherwise.
  def name
    (sprefix, *ssuffixes) = is_alpha? ? ["alpha", suffix] : [nil, "rc", suffix]
    [sprefix, "#{major}.#{minor}.#{patch}", *ssuffixes].compact.join('-')
  end
  …
  # And a lot of instance methods to bump versions up and down, etc
end

AliSoftware avatar Feb 02 '21 16:02 AliSoftware

Another benefit of refactoring this to be objects, and to have instance methods on them, is that it would be easier to refactor that to not rely on the various env vars like ENV['XX_CONFIG_FILE'], and to instead take the paths for those config files as parameter to the constructor of those Version objects. Which would then allow us to easily call both getters and setters (get current version, set new version for upcoming beta or release, etc) on the instance without relying to any global state

AliSoftware avatar May 14 '21 16:05 AliSoftware

Some work have already been started in the 203-version branch here to work on this refactoring.

Currently, only the model for Android versioning have been started to be worked on (and haven't been fully tested), so there's still work to do in order to apply the same ideas for iOS version objects.

This migration/refactoring would also make it easier to support multiple apps, in particular easily support both WordPress and Jetpack in the same repository, by passing the right parameters (Jetpack or WP config file paths) when constructing the VersionSet instance, at which point we could treat the VersionSet the same both for Jetpack and WordPress (calls to get and set version, etc) after the init point

AliSoftware avatar May 14 '21 17:05 AliSoftware