-
Notifications
You must be signed in to change notification settings - Fork 9
Add custom prefix configuration to DerivedBuildCodeFormatter
#656
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 8 commits
422c953
f11a2cc
49edcb6
75f4127
be13bee
4f36dee
4326bcc
31f6b73
bae9e25
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,9 +6,19 @@ module Versioning | |
| # The `DerivedBuildCodeFormatter` class is a specialized build code formatter for derived build codes. | ||
| # It takes in an AppVersion object and derives a build code from it. | ||
| 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. | ||
| # | ||
| def initialize(prefix: '1') | ||
| prefix ||= '' | ||
AliSoftware marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| validate_prefix!(prefix) | ||
| @prefix = prefix.to_s | ||
iangmaia marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| end | ||
|
|
||
| # Calculate the next derived build code. | ||
| # | ||
| # This method derives a new build code from the given AppVersion object by concatenating the digit 1, | ||
| # This method derives a new build code from the given AppVersion object by concatenating the configured prefix, | ||
| # the major version, the minor version, the patch version, and the build number. | ||
| # | ||
| # @param [AppVersion] version The AppVersion object to derive the next build code from. | ||
|
|
@@ -19,15 +29,43 @@ class DerivedBuildCodeFormatter | |
| # @return [String] The formatted build code string. | ||
| # | ||
| def build_code(build_code = nil, version:) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably not for this PR, but mentioning while I saw it: Note that this But in practice I just realized that:
That makes me think that (1) either one day we should create an abstract class for the Also, in any case, I am surprised that
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Oh, agreed.
True 🤔 |
||
| format( | ||
| # 1 is appended to the beginning of the string in case there needs to be additional platforms or | ||
| # extensions that could then use a different digit prefix such as 2, etc. | ||
| '1%<major>.2i%<minor>.2i%<patch>.2i%<build_number>.2i', | ||
| result = format( | ||
| # The prefix is configurable to allow for additional platforms or | ||
| # extensions that could use a different digit prefix such as 2, etc. | ||
| '%<prefix>s%<major>.2i%<minor>.2i%<patch>.2i%<build_number>.2i', | ||
| prefix: @prefix, | ||
| major: version.major, | ||
| minor: version.minor, | ||
| patch: version.patch, | ||
| build_number: version.build_number | ||
| ) | ||
|
|
||
| result.gsub(/^0+/, '') | ||
| end | ||
|
|
||
| private | ||
|
|
||
| # Validates that the prefix is a valid single digit (0-9) or empty string. | ||
| # | ||
| # @param [String] prefix The prefix to validate | ||
| # | ||
| # @raise [StandardError] If the prefix is invalid | ||
| # | ||
| def validate_prefix!(prefix) | ||
| prefix_str = prefix.to_s | ||
|
|
||
| # Allow empty string | ||
| return if prefix_str.empty? | ||
|
|
||
| # Check if it's longer than 1 character | ||
| if prefix_str.length > 1 | ||
| UI.user_error!("Prefix must be a single digit or empty string, got: '#{prefix_str}' (length: #{prefix_str.length})") | ||
| end | ||
|
|
||
| # Check if it's a valid integer | ||
| return if ('0'..'9').include?(prefix_str) | ||
|
|
||
| UI.user_error!("Prefix must be an integer digit (0-9) or empty string, got: '#{prefix_str}'") | ||
| end | ||
| end | ||
| end | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,16 +4,118 @@ | |
|
|
||
| describe Fastlane::Wpmreleasetoolkit::Versioning::DerivedBuildCodeFormatter do | ||
| describe 'derives a build code from an AppVersion object' do | ||
| it 'derives the build code from version numbers that are single digits' do | ||
| version = Fastlane::Models::AppVersion.new(1, 2, 3, 4) | ||
| build_code_string = described_class.new.build_code(version: version) | ||
| expect(build_code_string.to_s).to eq('101020304') | ||
| context 'with default prefix (backward compatibility)' do | ||
| it 'derives the build code from version numbers that are single digits' do | ||
| version = Fastlane::Models::AppVersion.new(1, 2, 3, 4) | ||
| build_code_string = described_class.new.build_code(version: version) | ||
| expect(build_code_string.to_s).to eq('101020304') | ||
| end | ||
|
|
||
| it 'derives the build code from version numbers that are two digits' do | ||
| version = Fastlane::Models::AppVersion.new(12, 34, 56, 78) | ||
| build_code_string = described_class.new.build_code(version: version) | ||
| expect(build_code_string.to_s).to eq('112345678') | ||
| end | ||
| end | ||
|
|
||
| context 'with explicit prefix' do | ||
| it 'derives the build code with prefix "1"' do | ||
| version = Fastlane::Models::AppVersion.new(1, 2, 3, 4) | ||
| formatter = described_class.new(prefix: '1') | ||
| build_code_string = formatter.build_code(version: version) | ||
| expect(build_code_string.to_s).to eq('101020304') | ||
| end | ||
|
|
||
| it 'derives the build code with prefix "2"' do | ||
| version = Fastlane::Models::AppVersion.new(1, 2, 3, 4) | ||
| formatter = described_class.new(prefix: '2') | ||
| build_code_string = formatter.build_code(version: version) | ||
| expect(build_code_string.to_s).to eq('201020304') | ||
| end | ||
|
|
||
| it 'derives the build code with prefix "0"' do | ||
| version = Fastlane::Models::AppVersion.new(12, 34, 56, 78) | ||
| formatter = described_class.new(prefix: '0') | ||
| build_code_string = formatter.build_code(version: version) | ||
| expect(build_code_string.to_s).to eq('12345678') | ||
| end | ||
| end | ||
|
|
||
| context 'with empty prefix' do | ||
| it 'derives the build code without prefix and trims leading zeros' do | ||
| version = Fastlane::Models::AppVersion.new(1, 2, 3, 4) | ||
| formatter = described_class.new(prefix: '') | ||
| build_code_string = formatter.build_code(version: version) | ||
| expect(build_code_string.to_s).to eq('1020304') | ||
| end | ||
|
|
||
| it 'derives the build code without prefix for two-digit major version' do | ||
| version = Fastlane::Models::AppVersion.new(12, 34, 56, 78) | ||
| formatter = described_class.new(prefix: '') | ||
| build_code_string = formatter.build_code(version: version) | ||
| expect(build_code_string.to_s).to eq('12345678') | ||
| end | ||
|
|
||
| it 'handles edge case with all zeros after removing leading zeros' do | ||
| version = Fastlane::Models::AppVersion.new(0, 0, 0, 1) | ||
| formatter = described_class.new(prefix: '') | ||
| build_code_string = formatter.build_code(version: version) | ||
| expect(build_code_string.to_s).to eq('1') | ||
| end | ||
| end | ||
|
|
||
| context 'with nil prefix' do | ||
| it 'treats nil prefix as empty string and derives build code without prefix' do | ||
| version = Fastlane::Models::AppVersion.new(1, 2, 3, 4) | ||
| formatter = described_class.new(prefix: nil) | ||
| build_code_string = formatter.build_code(version: version) | ||
| expect(build_code_string.to_s).to eq('1020304') | ||
| end | ||
|
|
||
| it 'treats nil prefix as empty string for two-digit major version' do | ||
| version = Fastlane::Models::AppVersion.new(12, 34, 56, 78) | ||
| formatter = described_class.new(prefix: nil) | ||
| build_code_string = formatter.build_code(version: version) | ||
| expect(build_code_string.to_s).to eq('12345678') | ||
| end | ||
| end | ||
| end | ||
|
|
||
| describe 'prefix validation' do | ||
| context 'with valid prefixes' do | ||
| it 'accepts single digits 0-9' do | ||
| (0..9).each do |digit| | ||
| expect { described_class.new(prefix: digit.to_s) }.not_to raise_error | ||
| end | ||
| end | ||
|
|
||
| it 'accepts empty string' do | ||
| expect { described_class.new(prefix: '') }.not_to raise_error | ||
| end | ||
|
|
||
| it 'accepts integer inputs' do | ||
| expect { described_class.new(prefix: 5) }.not_to raise_error | ||
| end | ||
| end | ||
|
|
||
| it 'derives the build code from version numbers that are two digits' do | ||
| version = Fastlane::Models::AppVersion.new(12, 34, 56, 78) | ||
| build_code_string = described_class.new.build_code(version: version) | ||
| expect(build_code_string.to_s).to eq('112345678') | ||
| context 'with invalid prefixes' do | ||
| it 'rejects multi-character strings' do | ||
| expect { described_class.new(prefix: '12') }.to raise_error(/Prefix must be a single digit or empty string/) | ||
| end | ||
|
|
||
| it 'rejects non-numeric strings' do | ||
| expect { described_class.new(prefix: 'a') }.to raise_error(/Prefix must be an integer digit/) | ||
| end | ||
|
|
||
| it 'rejects numbers outside 0-9 range' do | ||
| expect { described_class.new(prefix: '10') }.to raise_error(/Prefix must be a single digit or empty string/) | ||
| expect { described_class.new(prefix: '-1') }.to raise_error(/Prefix must be a single digit or empty string/) | ||
| end | ||
|
|
||
| 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 | ||
|
Comment on lines
+115
to
+118
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👌 (this reminded me of this meme 😄 ) |
||
| end | ||
| end | ||
| end | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized that funnily enough, I'm not even sure any of our projects use this
DerivedBuildCodeFormatteryet 🤔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)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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 itThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in bae9e25