feat: Semantic Versioning suppport in Audience Evaluation #213

yasirfolio3 · 2020-08-28T18:04:13Z

Summary

add greater than or equal and less than or equal for numbers
add semantic version match types.

…perator.

coveralls · 2020-08-28T18:19:58Z

Coverage decreased (-0.1%) to 98.156% when pulling 7ac685a on yasir/sem-ver into 543a8e3 on master.

oakbani

Some changes requested.

oakbani · 2020-08-31T08:00:26Z

src/Optimizely/Utils/CustomAttributeConditionEvaluator.php

+    protected function semverEqualEvaluator($condition)
+    {
+        $comparison = $this->semverEvaluator($condition);
+        if ($comparison !== null) {


nit. Will be more readable if you do:
if ($comparison === null) { return null } return $comparison === 0
For all others too

oakbani · 2020-08-31T08:07:18Z

tests/UtilsTests/CustomAttributeConditionEvaluatorTest.php

+
+    public function testSemVerGTMatcherReturnsFalseWhenAttributeValueIsLessThanOrEqualToConditionValue()
+    {
+        $customAttrConditionEvaluator = new CustomAttributeConditionEvaluator(


nit. define the condition inside this method just like you have done below. testSemVerGTMatcherReturnsTrueWhenAttributeValueIsGreaterThanConditionValueBeta()

oakbani · 2020-08-31T08:15:01Z

tests/UtilsTests/CustomAttributeConditionEvaluatorTest.php

+        $invalidValues = ["-", ".", "..", "+", "+test", " ", "2 .3. 0", "2.",
+            ".2.2", "3.7.2.2", "3.x", ",", "+build-prerelease"];
+
+        foreach ($invalidValues as $val) {


can you write the above tests in a for loop just like you have done here?
seta custom error message and send it to assert call as last argument.

oakbani

Rather than creating SemVersionConditionEvaluator as a separate class and creating it's instance on every evaluation call. Make method inside SemVerionEvaluator static. And directly use them from different evaluator methods. Pass logger as a param to compare_version static method. And move string validation to compare_version method as well.

compare_version should get (target_version, user_version, logger)

oakbani · 2020-09-04T07:38:46Z

src/Optimizely/Utils/CustomAttributeConditionEvaluator.php

+        $conditionValue = $condition['value'];
+        $userValue = isset($this->userAttributes[$conditionName]) ? $this->userAttributes[$conditionName] : null;
+
+        if (!Validator::validateNonEmptyString($conditionValue)) {


$userValue should be checked here instead of $conditionValue. And a unit test written if not already.

oakbani · 2020-09-04T07:49:34Z

src/Optimizely/Utils/SemVersionConditionEvaluator.php

+    private function toInt($str)
+    {
+        if (is_numeric($str)) {
+            return intval($str);


I think you can directly use PHP intVal method. https://www.php.net/manual/en/function.intval.php

The integer value of var on success, or 0 on failure. Empty arrays return 0, non-empty arrays return 1.

oakbani · 2020-09-04T07:51:08Z

src/Optimizely/Utils/SemVersionConditionEvaluator.php

+    }
+
+    /**
+     * checks if string contains prerelease seperator before build separator.


Checks if given string is a prerelease version?

prerelease seperator before build separator. is an implementation detail that should come as a comment inside code.

oakbani · 2020-09-04T07:51:24Z

src/Optimizely/Utils/SemVersionConditionEvaluator.php

+    /**
+     * checks if string contains prerelease seperator before build separator.
+     *
+     * @param  string $str value to be checked.


let's rename $str to $version

oakbani · 2020-09-04T07:51:40Z

src/Optimizely/Utils/SemVersionConditionEvaluator.php

+     *
+     * @param  string $str value to be checked.
+     *
+     * @return bool true if prerelease seperator comes first.


true if given version is a prerelease

oakbani · 2020-09-04T07:58:52Z

src/Optimizely/Utils/SemVersionConditionEvaluator.php

+     * @return null|array   array if provided string was successfully split,
+     *                      null if any issues occured.
+     */
+    protected function splitSemanticVersion($targetedVersion)


nit.rename to $version. This method doesn't know if it's targeted or userVersion

oakbani · 2020-09-04T08:00:29Z

src/Optimizely/Utils/SemVersionConditionEvaluator.php

+        $targetSuffix = array();
+
+        if ($this->isPreRelease($targetedVersion) || $this->isBuild($targetedVersion)) {
+            $targetParts = explode($this->isPreRelease($targetPrefix) ? self::PRE_RELEASE_SEPARATOR : self::BUILD_SEPARATOR, $targetedVersion, 2);


$this->isPreRelease($targetPrefix) ? self::PRE_RELEASE_SEPARATOR : self::BUILD_SEPARATOR assign this to a variable before use. Readability.

oakbani · 2020-09-04T08:02:50Z

src/Optimizely/Utils/SemVersionConditionEvaluator.php

+        if ($targetedVersionParts === null) {
+            return null;
+        }
+        $versionParts = $this->splitSemanticVersion($this->condition);


nit. rename userVersionParts

oakbani · 2020-09-04T08:03:44Z

src/Optimizely/Utils/SemVersionConditionEvaluator.php

+
+        // Up to the precision of targetedVersion, expect version to match exactly.
+        for ($i = 0; $i < count($targetedVersionParts); $i++) {
+            if (count($versionParts) <= $i) {


count($versionParts) assign this to a variable at top and resuse

oakbani · 2020-09-04T08:04:29Z

src/Optimizely/Utils/SemVersionConditionEvaluator.php

+        // Up to the precision of targetedVersion, expect version to match exactly.
+        for ($i = 0; $i < count($targetedVersionParts); $i++) {
+            if (count($versionParts) <= $i) {
+                // even if they are equal at this point. if the target is a prerelease then it must be greater than the pre release.


nit. looks like there is something wrong with this comment.

oakbani

Looks good.

oakbani · 2020-09-04T13:50:18Z

src/Optimizely/Utils/CustomAttributeConditionEvaluator.php

@@ -143,15 +143,13 @@ protected function semverEvaluator($condition)
        $conditionValue = $condition['value'];
        $userValue = isset($this->userAttributes[$conditionName]) ? $this->userAttributes[$conditionName] : null;

-        if (!Validator::validateNonEmptyString($conditionValue)) {
+        if (!Validator::validateNonEmptyString($conditionValue) || !Validator::validateNonEmptyString($userValue)) {


just remove the check for conditionValue. We assume that we get string semantic version value from the datafile/backend.
And in case of bad condition value, the error message being logged is misleading.

nit.
Modify doc string of this method to returns result of SemVersionConditionEvaluator::compareVersion for given target and user versions.

oakbani

@yasirfolio3 Code Optimization

oakbani · 2020-09-04T17:19:57Z

src/Optimizely/Utils/SemVersionConditionEvaluator.php

+        $targetSuffix = array();
+
+        if (self::isPreRelease($version) || self::isBuild($version)) {
+            $separator = self::isPreRelease($targetPrefix) ? self::PRE_RELEASE_SEPARATOR : self::BUILD_SEPARATOR;


self::isPreRelease($targetPrefix)
nit. do this once
if (self::isPreRelease)
$separator = ...
elseif(self::isBuild)
$separator = ...

oakbani · 2020-09-04T17:22:25Z

src/Optimizely/Utils/SemVersionConditionEvaluator.php

+        if (!self::isPreRelease($targetedVersion) && self::isPreRelease($userVersion)) {
+            return -1;
+        }
+        return 0;


assign self::isPreRelease($targetedVersion) and self::isPreRelease($userVersion) to variables at the top of this method. And reuse them at all places.

aliabbasrizvi · 2020-09-08T18:57:55Z

src/Optimizely/Utils/CustomAttributeConditionEvaluator.php

+        return array(self::EXACT_MATCH_TYPE, self::EXISTS_MATCH_TYPE, self::GREATER_THAN_MATCH_TYPE, self::GREATER_THAN_EQUAL_TO_MATCH_TYPE,
+            self::LESS_THAN_MATCH_TYPE, self::LESS_THAN_EQUAL_TO_MATCH_TYPE, self::SUBSTRING_MATCH_TYPE, self::SEMVER_EQ,
+            self::SEMVER_GT, self::SEMVER_GE, self::SEMVER_LT, self::SEMVER_LE);


Can this be made nicer by having these on separate lines and alphabetized.

aliabbasrizvi · 2020-09-08T18:58:14Z

src/Optimizely/Utils/CustomAttributeConditionEvaluator.php

        $evaluatorsByMatchType[self::SUBSTRING_MATCH_TYPE] = 'substringEvaluator';
+        $evaluatorsByMatchType[self::SEMVER_EQ] = 'semverEqualEvaluator';


nit. Alphabetize.

aliabbasrizvi · 2020-09-08T18:58:36Z

src/Optimizely/Utils/CustomAttributeConditionEvaluator.php

@@ -109,6 +125,33 @@ protected function isValueTypeValidForExactConditions($value)
        return false;
    }

+    /**
+     * returns result of SemVersionConditionEvaluator::compareVersion for given target and user versions.


nit. Returns

aliabbasrizvi · 2020-09-08T18:59:27Z

src/Optimizely/Utils/CustomAttributeConditionEvaluator.php

+     * @return null|int 0 if user's semver attribute is equal to the semver condition value,
+     *                  1 if user's semver attribute is greater than the semver condition value,
+     *                  -1 if user's semver attribute is less than the semver condition value,


User has a version attribute. Semver is a specification.

aliabbasrizvi · 2020-09-08T19:00:26Z

src/Optimizely/Utils/CustomAttributeConditionEvaluator.php

+     *
+     * @param  object $condition
+     *
+     * @return boolean true if user's semver attribute is greater than the semver condition value,


nit. version attribute. Please update at all places.

aliabbasrizvi · 2020-09-08T19:01:57Z

src/Optimizely/Utils/SemVersionConditionEvaluator.php

+    /**
+     * compares targeted version with the provided user version.
+     *
+     * @param  object $targetedVersion


Aren't the versions string?

src/Optimizely/Utils/SemVersionConditionEvaluator.php

aliabbasrizvi · 2020-09-08T20:30:49Z

src/Optimizely/Utils/SemVersionConditionEvaluator.php

+     *
+     * @return bool true if given version is a build.
+     */
+    private static function isBuild($version)


nit. isBuildVersion

aliabbasrizvi · 2020-09-08T20:31:01Z

src/Optimizely/Utils/SemVersionConditionEvaluator.php

+     *
+     * @return bool true if given version is a prerelease.
+     */
+    private static function isPreRelease($version)


nit. isPreReleaseVersion

aliabbasrizvi · 2020-09-14T16:24:18Z

src/Optimizely/Utils/SemVersionConditionEvaluator.php

+     * @param  string $userVersion
+     * @param  object $logger
+     *
+     * @return null|int 0 if user's semver attribute is equal to the semver condition value,


nit. user's attribute or user's version attribute

Please apply this feedback throughout the PR.

aliabbasrizvi · 2020-09-14T16:24:55Z

src/Optimizely/Utils/SemVersionConditionEvaluator.php

+     * @param  string $userVersion
+     * @param  object $logger
+     *
+     * @return null|int 0 if user's semver attribute is equal to the semver condition value,


Please apply this feedback throughout the PR.

msohailhussain

lgtm

yasirfolio3 added 6 commits August 25, 2020 17:55

Initial commit.

88dffb7

Tests added for ge and le.

ea29804

linter warning fixed.

9f1bf1c

Unit tests implemented.

6a0d5d3

warnings fixed.

41e373b

Added logging tests and fixed issue of handling multiple '-' or '+' o…

e5f1c45

…perator.

yasirfolio3 requested a review from msohailhussain August 28, 2020 18:04

optimizely-codereview-bot assigned yasirfolio3 Aug 28, 2020

fixes.

5880e51

msohailhussain closed this Aug 28, 2020

msohailhussain reopened this Aug 28, 2020

yasirfolio3 closed this Aug 28, 2020

yasirfolio3 reopened this Aug 28, 2020

oakbani reviewed Aug 31, 2020

View reviewed changes

yasirfolio3 and others added 5 commits September 1, 2020 14:08

Suggested changes made.

cc7d95b

suggested change made.

ece7e62

Merge branch 'master' into yasir/sem-ver

f7bfdf6

fixes.

ee3f9f2

Merge branch 'master' into yasir/sem-ver

cda1a90

oakbani changed the title ~~feature: add semantic version types~~ feat: Semantic Versioning suppport in Audience Evaluation Sep 4, 2020

oakbani suggested changes Sep 4, 2020

View reviewed changes

suggested changes made.

7c61ebe

yasirfolio3 requested a review from oakbani September 4, 2020 10:32

yasirfolio3 added 2 commits September 4, 2020 15:44

nit fixed.

5c70a89

fixes.

0cdb46b

oakbani approved these changes Sep 4, 2020

View reviewed changes

oakbani reviewed Sep 4, 2020

View reviewed changes

yasirfolio3 added 2 commits September 8, 2020 17:52

suggested changes made.

761d679

fixes.

137b967

yasirfolio3 marked this pull request as ready for review September 8, 2020 15:15

yasirfolio3 requested a review from a team as a code owner September 8, 2020 15:15

aliabbasrizvi reviewed Sep 8, 2020

View reviewed changes

fixes.

fac2ec2

yasirfolio3 removed their assignment Sep 9, 2020

optimizely-codereview-bot requested review from aliabbasrizvi and oakbani September 9, 2020 07:43

aliabbasrizvi reviewed Sep 14, 2020

View reviewed changes

aliabbasrizvi approved these changes Sep 14, 2020

View reviewed changes

suggested fix made.

7ac685a

msohailhussain closed this Sep 14, 2020

msohailhussain reopened this Sep 14, 2020

optimizely-codereview-bot assigned yasirfolio3 Sep 14, 2020

msohailhussain closed this Sep 14, 2020

msohailhussain reopened this Sep 14, 2020

msohailhussain unassigned yasirfolio3 Sep 14, 2020

msohailhussain approved these changes Sep 15, 2020

View reviewed changes

mikeproeng37 merged commit d0c6f5e into master Sep 15, 2020

mikeproeng37 deleted the yasir/sem-ver branch September 15, 2020 04:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Semantic Versioning suppport in Audience Evaluation #213

feat: Semantic Versioning suppport in Audience Evaluation #213

yasirfolio3 commented Aug 28, 2020

coveralls commented Aug 28, 2020 •

edited

Loading

oakbani left a comment

oakbani Aug 31, 2020

oakbani Aug 31, 2020

oakbani Aug 31, 2020

oakbani left a comment

oakbani Sep 4, 2020

oakbani Sep 4, 2020

oakbani Sep 4, 2020

oakbani Sep 4, 2020

oakbani Sep 4, 2020

oakbani Sep 4, 2020

oakbani Sep 4, 2020

oakbani Sep 4, 2020

oakbani Sep 4, 2020

oakbani Sep 4, 2020

oakbani left a comment

oakbani Sep 4, 2020

oakbani Sep 4, 2020

oakbani left a comment

oakbani Sep 4, 2020

oakbani Sep 4, 2020

aliabbasrizvi Sep 8, 2020

aliabbasrizvi Sep 8, 2020

aliabbasrizvi Sep 8, 2020

aliabbasrizvi Sep 8, 2020

aliabbasrizvi Sep 8, 2020

aliabbasrizvi Sep 8, 2020

aliabbasrizvi Sep 8, 2020

aliabbasrizvi Sep 8, 2020

aliabbasrizvi Sep 14, 2020

aliabbasrizvi Sep 14, 2020

aliabbasrizvi Sep 14, 2020

msohailhussain left a comment

		$evaluatorsByMatchType[self::SUBSTRING_MATCH_TYPE] = 'substringEvaluator';
		$evaluatorsByMatchType[self::SEMVER_EQ] = 'semverEqualEvaluator';

feat: Semantic Versioning suppport in Audience Evaluation #213

feat: Semantic Versioning suppport in Audience Evaluation #213

Conversation

yasirfolio3 commented Aug 28, 2020

Summary

coveralls commented Aug 28, 2020 • edited Loading

oakbani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oakbani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oakbani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oakbani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msohailhussain left a comment

Choose a reason for hiding this comment

coveralls commented Aug 28, 2020 •

edited

Loading