-
Notifications
You must be signed in to change notification settings - Fork 8
Add version components digits configuration to DerivedBuildCodeFormatter #657
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
Conversation
0a9e9e0 to
09e71f9
Compare
…gmaia/derived-build-formatter-var-digits
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
Outdated
Show resolved
Hide resolved
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
Outdated
Show resolved
Hide resolved
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
Outdated
Show resolved
Hide resolved
AliSoftware
left a comment
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.
Mostly 💄 nitpicks + some suggestions to improve the unit tests. Otherwise LGTM 👍
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
Outdated
Show resolved
Hide resolved
lib/fastlane/plugin/wpmreleasetoolkit/versioning/formatters/derived_build_code_formatter.rb
Outdated
Show resolved
Hide resolved
Co-authored-by: Olivier Halligon <[email protected]>
Co-authored-by: Olivier Halligon <[email protected]>
Co-authored-by: Olivier Halligon <[email protected]>
Co-authored-by: Olivier Halligon <[email protected]>
|
@AliSoftware I believe I addressed all the test suggestions! |
AliSoftware
left a comment
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.
Just nitpicks to fix code comments, otherwise LGTM so approving to unblock you
| def validate_prefix!(prefix) | ||
| unless prefix.nil? || prefix.is_a?(String) || prefix.is_a?(Integer) | ||
| UI.user_error!("Prefix must be a string or integer, got: #{prefix.class}") | ||
| end | ||
|
|
||
| prefix_str = prefix.to_s |
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.
👍 Today I re-learned that nil.to_s returns the '' empty string (as opposed to crashing).
This is why even if now you call validate_prefix! from the def initialize without doing the prefix ||= '' anymore first, this prefix_str = prefix.to_s still doesn't crash 👍
|
|
||
| describe 'digit count validation' do | ||
| # Generate all combinations of [1,2,3] for each of the 4 digit count parameters | ||
| all_digit_combinations = Array.new(4, [1, 2, 3]).reduce(&:product).map(&:flatten) |
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'm surprised that rubocop doesn't complain about you using a local variable here instead of using RSpec's let(:all_digit_combinations) { … } syntax (which I believed was more idiomatic to Rspec)…
I don't mind either approach in practice (should do the same in the end, maybe except some memoizing of the value when using let?). I just wouldn't want a rubocop warning to appear because of this?
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 I did it first with a let, but had to re-learn that it won't work if we use it as part of the context:
`all_digit_combinations` is not available on an example group (e.g. a `describe` or `context` block). It is only available from within individual examples (e.g. `it` blocks) or from constructs that run in the scope of an example (e.g. `before`, `let`, etc).
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
lib/fastlane/plugin/wpmreleasetoolkit/versioning/formatters/derived_build_code_formatter.rb
Show resolved
Hide resolved
Co-authored-by: Olivier Halligon <[email protected]>
Co-authored-by: Olivier Halligon <[email protected]>
Co-authored-by: Olivier Halligon <[email protected]>
What does it do?
This PR adds to the
DerivedBuildCodeFormatterclass the possibility to configure a variable number of digits per version components, so that we have more flexibility to increase the maximum number of builds per a specific major / minor version, for example.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.