-
Notifications
You must be signed in to change notification settings - Fork 19
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
allow no prefix + pass credo #11
Conversation
Ebert has finished reviewing this Pull Request and has found:
You can see more details about this review at https://ebertapp.io/github/zachdaniel/git_ops/pulls/11. |
Pull Request Test Coverage Report for Build 75
💛 - Coveralls |
lib/mix/tasks/git_ops.release.ex
Outdated
@@ -100,10 +106,13 @@ defmodule Mix.Tasks.GitOps.Release do | |||
) | |||
end | |||
|
|||
new_version = String.trim_leading(prefixed_new_version, prefix) | |||
new_version = | |||
if "" != prefix, |
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 think, purely stylistically, I'd rather see this as:
new_version =
if prefix != "" do
String.trim_leading(prefixed_new_version, prefix)
else
prefixed_new_version
end
Thank you so much for all the cleanup you did in addition to this PR. Great stuff! I had one stylistic piece of feedback, but aside from that this looks great. I'll approve/merge once that change is made (or if you disagree with it feel free to discuss). |
One more thing, if you could just remove the version change, as well as changes to the changelog. My workflow is to do the release and generate the changelog after merging the PR. Thanks! |
done |
Contributor checklist
For example:
fix: Multiply by appropriate coefficient
, orfeat(Calculator): Correctly preserve history
Any explanation or long form information in your commit message should be
in a separate paragraph, separated by a blank line from the primary message