8000 fix: check pre-release and build versions by oakbani · Pull Request #269 · optimizely/ruby-sdk · GitHub
[go: up one dir, main page]
Skip to content

fix: check pre-release and build versions # 8000 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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Sep 8, 2020
Merged

fix: check pre-release and build versions #269

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 51 additions & 11 deletions lib/optimizely/semantic_version.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,43 @@ module SemanticVersion
module_function

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
Comment on lines 29 to +66
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor
@thomaszurkan-optimizely thomaszurkan-optimizely Sep 8, 2020 •
edited
Loading

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.

Copy link
Contributor

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.

end

def split_semantic_version(target)
Expand All @@ -52,9 +81,9 @@ def split_semantic_version(target)
raise InvalidSemanticVersion if target.include? ' '

if pre_release?(target)
target_parts = target.split(SEMVER_PRE_RELEASE)
elsif target.include? SEMVER_BUILD
target_parts = target.split(SEMVER_BUILD)
target_parts = target.split(SEMVER_PRE_RELEASE, 2)
elsif build? target
target_parts = target.split(SEMVER_BUILD, 2)
end

unless target_parts.empty?
Expand Down Expand Up @@ -91,6 +120,9 @@ def compare_user_version_with_target_version(target_version, user_version)
raise InvalidAttributeType unless target_version.is_a? String
raise InvalidAttributeType unless user_version.is_a? String

is_target_version_prerelease = pre_release?(target_version)
is_user_version_prerelease = pre_release?(user_version)

target_version_parts = split_semantic_version(target_version)
user_version_parts = split_semantic_version(user_version)
user_version_parts_len = user_version_parts.length if user_version_parts
Expand All @@ -99,15 +131,23 @@ def compare_user_version_with_target_version(target_version, user_version)
target_version_parts.each_with_index do |_item, idx|
if user_version_parts_len <= idx
# even if they are equal at this point. if the target is a prerelease
# then it must be greater than the pre release.
return 1 if pre_release?(target_version)
# then user version must be greater than the pre release.
return 1 if is_target_version_prerelease

return -1

elsif !Helpers::Validator.string_numeric? user_version_parts[idx]
# compare strings
return -1 if user_version_parts[idx] < target_version_parts[idx]
return 1 if user_version_parts[idx] > target_version_parts[idx]
if user_version_parts[idx] < target_version_parts[idx]
return 1 if is_target_version_prerelease && !is_user_version_prerelease

return -1

elsif user_version_parts[idx] > target_version_parts[idx]
return -1 if is_user_version_prerelease && !is_target_version_prerelease

return 1
end

else
user_version_part = user_version_parts[idx].to_i
Expand All @@ -118,7 +158,7 @@ def compare_user_version_with_target_version(target_version, user_version)
end
end

return -1 if pre_release?(user_version) && !pre_release?(target_version)
return -1 if is_user_version_prerelease && !is_target_version_prerelease

0
end
Expand Down
14 changes: 10 additions & 4 deletions spec/semantic_version_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@
['2.1', '2.1.0'],
['2.1', '2.1.215'],
['2', '2.12'],
['2', '2.785.13']
['2', '2.785.13'],
['3.7.0', '3.7.0+build']
]

versions.each do |target_version, user_version|
Expand All @@ -49,7 +50,10 @@
['2.9.0', '2.9.1'],
['2.1.2', '2.1.3-beta'],
['2.1.2-beta', '2.1.2-release'],
['2.1.3-beta', '2.1.3']
['2.1.3-beta', '2.1.3'],
['2.1.3-beta+1', '2.1.3-beta+1.2.3'],
['3.7.0-prerelease', '3.7.0+build'],
['3.7.0-prerelease+build', '3.7.0-prerelease-prelrease+rc']
]

versions.each do |target_version, user_version|
Expand All @@ -69,7 +73,9 @@
['2.3.5', '2.3.1'],
['2.9.8', '2.9'],
['2.1.2-release', '2.1.2-beta'],
['2.1.3', '2.1.3-beta']
['2.1.3', '2.1.3-beta'],
['2.1.3+build-1.2.3', '2.1.3+build-1'],
['3.7.0', '3.7.0-prerelease']
]

versions.each do |target_version, user_version|
Expand All @@ -83,7 +89,7 @@

# invalid semantic version
versions = [
'-', '.', '..', '+', '+test', ' ', '2 .3. 0', '2.', '.2.2', '3.7.2.2', '3.x', ',', '+build-prerelease'
'-', '.', '..', '+', '+test', ' ', '2 .3. 0', '2.', '.2.2', '3.7.2.2', '3.x', ',', '+build-prerelease', '2..2'
]

versions.each do |user_version|
Expand Down
0