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
changelog_from_git_commits fix from command line #12477
changelog_from_git_commits fix from command line #12477
Conversation
@joshdholtz This is ready for review. 👀 (getting my emoji game strong 💪 ) 😆 |
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.
A few comments to make sure unnecessary crashes don't happen based on data types and content. We should maybe add some tests for when data is bad? Ex: when a string is based in an there isn't a comma
UI.user_error!(":between must be of type array") unless value.kind_of?(Array) | ||
UI.user_error!(":between must not contain nil values") if value.any?(&:nil?) | ||
UI.user_error!(":between must be an array of size 2") unless (value || []).size == 2 | ||
unless value.kind_of?(String) |
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.
Can we verify if its a string that it has a comma in it?
@@ -10,7 +10,11 @@ def self.run(params) | |||
UI.success("Collecting the last #{params[:commits_count]} Git commits") | |||
else | |||
if params[:between] | |||
from, to = params[:between] | |||
if params[:between].include?(",") # running from shell |
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.
We might need check if its a string here before calling include
?
from, to = params[:between] | ||
if params[:between].include?(",") # running from shell | ||
from, to = params[:between].split(",", 2) | ||
else |
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.
And depending on how the above is structured.. we may need to add a check to make sure that this is an array?
@joshdholtz all the comments are addressed. Ready to review again. 👍 |
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.
This is golden! Thanks for the contribution ❤️
Just wanted to explain what we are doing here incase. The between option now takes both array of string and string with comma in it. |
Nice one @RishabhTayal! |
Hey @RishabhTayal 👋 Thank you for your contribution to fastlane and congrats on getting this pull request merged 🎉 Please let us know if this change requires an immediate release by adding a comment here 👍 |
Congratulations! 🎉 This was released as part of fastlane 2.96.0 🚀 |
fixes: #10894