fix: check pre-release and build versions #269
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.
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.
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.
@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.
Sure. I accept that it is small. Ok in current form.
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