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 #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

Merged
merged 1 commit into from
Sep 8, 2020

Conversation

oakbani
Copy link
Contributor
@oakbani oakbani commented Sep 4, 2020

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

@oakbani oakbani requested a review from a team as a code owner September 4, 2020 15:26
@oakbani oakbani removed their assignment Sep 4, 2020
@coveralls
Copy link

Coverage Status

Coverage increased (+0.001%) to 99.584% when pulling 4850438 on oakbani/semver-2 into bc09647 on master.

Comment on lines 29 to +66
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
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.

8000
Copy link
Contributor
@thomaszurkan-optimizely thomaszurkan-optimizely Sep 8, 2020

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.

Copy link
Contributor
@thomaszurkan-optimizely thomaszurkan-optimizely left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thomaszurkan-optimizely thomaszurkan-optimizely merged commit 84c4397 into master Sep 8, 2020
@thomaszurkan-optimizely thomaszurkan-optimizely deleted the oakbani/semver-2 branch September 8, 2020 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants
0