Add custom prefix configuration to DerivedBuildCodeFormatter
#656
Conversation
| # Only trim leading zeros when prefix is empty (to avoid removing explicit "0" prefix) | ||
| if @prefix.empty? | ||
| result.gsub(/^0+/, '') | ||
| else | ||
| result | ||
| end |
There was a problem hiding this comment.
I'm actually not sure how the Google Play API would react to submitting a build with a versionCode that has a leading zero 🤔
There was a problem hiding this comment.
iinm, it needs to be an integer in the API so I'd guess they would just be ignored? 🤔 But also considering iOS, perhaps it doesn't make a lot of sense to allow for 0 to be used as prefix (or maybe we should treat it the same as '').
There was a problem hiding this comment.
Yeah I think doing the gsub unconditionally should be fine.
| # Check if it's within valid range (0-9) | ||
| return if prefix_int.between?(0, 9) | ||
|
|
||
| UI.user_error!("Prefix must be a single digit (0-9), got: #{prefix_int}") |
There was a problem hiding this comment.
I'm not sure how it'd be technically possible to get to that point in the code and fail that test, given we already ensured that prefix_str would not be a string longer than 1 and would be a valid Integer 😅 🙃
lib/fastlane/plugin/wpmreleasetoolkit/versioning/formatters/derived_build_code_formatter.rb
Outdated
Show resolved
Hide resolved
lib/fastlane/plugin/wpmreleasetoolkit/versioning/formatters/derived_build_code_formatter.rb
Show resolved
Hide resolved
| # | ||
| # @return [String] The formatted build code string. | ||
| # | ||
| def build_code(build_code = nil, version:) |
There was a problem hiding this comment.
Probably not for this PR, but mentioning while I saw it:
Note that this build_code parameter is unused, but was initially still kept here so that the common API for all the *BuildCodeFormatter have the same API (as mentioned in the YARD comment about it), making it easier to switch from one formatter to another without changing the call sites (abstraction layer pattern).
But in practice I just realized that:
- We don't have an abstract class for all the
*BuildCodeFormatterclasses like we have forAbstractVersionFormatter. In fact, the*BuildCodeFormatterclasses don't inherit any common parent class, unlike the*VersionFormatterclasses - In practice the API for all
*BuildCodeFormatterclasses is not even consistent (which might actually explain point 1). For example thisDerivedBuildCodeFormatteras well asFourPartBuildCodeFormatterindeed both have a signature ofdef build_code(build_code = nil, version:)… but theSimpleBuildCodeFormatterusesdef build_code(version = nil, build_code:)instead…
That makes me think that (1) either one day we should create an abstract class for the *BuildCodeFormatter and unify their API for consistency (2) or we should just abandon the idea of having unified API for all (given the needed input parameters might depend on what the formatter does and needs to compute its value), and remove the unused parameters from the def build_code(…) signatures for clarity… 🙃
Also, in any case, I am surprised that rubocop didn't complain about it and didn't suggest us to rename this parameter with an underscore to indicate that it being unused is intentional (def build_code(_build_code = nil, version:) 🤷
There was a problem hiding this comment.
That makes me thing that (1) either one day we should create an abstract class for the *BuildCodeFormatter and unify their API for consistency (2) or we should just abandon the idea of having unified API for all
Oh, agreed.
I quickly saw the parameter and imagined there would be a base, abstract class like the other versioning formatters.
I am surprised that rubocop didn't complain about it and didn't suggest us to rename this parameter
True 🤔
| it 'rejects symbols and special characters' do | ||
| expect { described_class.new(prefix: '@') }.to raise_error(/Prefix must be an integer digit/) | ||
| expect { described_class.new(prefix: '#') }.to raise_error(/Prefix must be an integer digit/) | ||
| end |
Co-authored-by: Olivier Halligon <[email protected]>
AliSoftware
left a comment
AliSoftware
left a comment
There was a problem hiding this comment.
Suggesting one last unit test but otherwise LGTM so approving to unblock.
lib/fastlane/plugin/wpmreleasetoolkit/versioning/formatters/derived_build_code_formatter.rb
Show resolved
Hide resolved
| class DerivedBuildCodeFormatter | ||
| # Initialize the formatter with a configurable prefix. | ||
| # | ||
| # @param [String] prefix The prefix to use for the build code. Must be a single digit (0-9), or the empty string. Defaults to '1' for backward compatibility. |
There was a problem hiding this comment.
I just realized that funnily enough, I'm not even sure any of our projects use this DerivedBuildCodeFormatter yet 🤔
- Tumblr uses derived
versionCodebased onversionName… but they have their own in-repo implementation (and we've been wanting to migrate to therelease-toolkitone instead for a while for consistency, but never found the time—see https://linear.app/a8c/issue/AINFRA-155/fix-tumblr-versioning-consistency) - I thought for a moment that PocketCasts used that as well because I remember them using some prefix in their
versionCodeto differentiate the Android app, Wear app and Automotive app… but looking at it that's still aSimpleBuildCodeFormatterincrementing the versions, and what I remembered was just an offset between the 3 form factors, not a prefix
… So in practice Gravatar-Android would be the first one to use that class iinm 🙃
So I wonder if it's really needed to default to '1' "for backwards compatibility" given this…
On one hand, better safe that sorry and we can always pass a different value at call site in the product's Fastfile. Oh the other hand, if we wanted to change that default prefix, it might actually be now or never 😅
There was a problem hiding this comment.
Yeah, that's true. At some point I also realized that Tumblr used their own implementation but wasn't sure about other apps... and anyway this would be a breaking change of the API technically.
But indeed, in practice it would be nicer to not have this default and avoid this issue in the future (especially if more apps adopt this DerivedBuildCodeFormatter). Shall we do it? I think I'll do it 
What does it do?
This PR adds to the class
DerivedBuildCodeFormatterthe possibility to configure a versioning prefix instead of always defaulting to1.Checklist before requesting a review
bundle exec rubocopto test for code style violations and recommendations.specs/*_spec.rb) if applicable.bundle exec rspecto run the whole test suite and ensure all your tests pass.CHANGELOG.mdfile to describe your changes under the appropriate existing###subsection of the existing## Trunksection.MIGRATION.mdfile to describe how the changes will affect the migration from the previous major version and what the clients will need to change and consider.