-
Notifications
You must be signed in to change notification settings - Fork 28
fix: check pre-release and build versions #269
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
def pre_release?(target) | ||
# Method to check if the given version contains "-" | ||
# Method to check if the given version is a prerelease | ||
# | ||
# target - String representing semantic version | ||
# | ||
# Returns true if the given version does contain "-" | ||
# Returns true if the given version is a prerelease | ||
# false if it doesn't | ||
|
||
target.include? SEMVER_PRE_RELEASE | ||
raise unless target.is_a? String | ||
|
||
prerelease_index = target.index(SEMVER_PRE_RELEASE) | ||
build_index = target.index(SEMVER_BUILD) | ||
|
||
return false if prerelease_index.nil? | ||
return true if build_index.nil? | ||
|
||
# when both operators are present prerelease should precede the build operator | ||
prerelease_index < build_index | ||
end | ||
|
||
def build?(target) | ||
# Method to check if the given version is a build | ||
# | ||
# target - String representing semantic version | ||
# | ||
# Returns true if the given version is a build | ||
# false if it doesn't | ||
|
||
raise unless target.is_a? String | ||
|
||
prerelease_index = target.index(SEMVER_PRE_RELEASE) | ||
build_index = target.index(SEMVER_BUILD) | ||
|
||
return false if build_index.nil? | ||
return true if prerelease_index.nil? | ||
|
||
# when both operators are present build should precede the prerelease operator | ||
build_index < prerelease_index |
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.
Have we considered just having a single method called prerelease_or_build
and that just determines if version is prerelease or build vs doing similar things twice.
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.
Calling the index method is not cheap and we can technically halve the number of times we call it.
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.
@aliabbasrizvi I would generally agree with you. However, you need to keep in context your context :D. Walking a small string like a version string (finding first index is going to be quite small (e.g. 2.0.0 or 2.0.0- or 2.0.0+). We could always just check the 6th element. But again, in general you are walking a tiny string.
Also, we need to determine between the two so we need both isPreRelease and isBuild.
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.
Sure. I accept that it is small. Ok in current form.
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.
LGTM
Summary
This PR adds some additional checks when figuring out if the given version is a build or a prerelease.
For a pre-release version . '-' should come before '+' ( if plus exists).
For a build version: '+' should come before '-' (if minus exists)
Test plan
Added unit tests
Issues