Amna ejaz/semver python #1

msohailhussain · 2020-07-16T17:17:00Z

Summary

The “what”; a concise description of each logical change
Another change

The “why”, or other context.

Test plan

Issues

“THING-1234” or “Fixes Overhaul logging integration. optimizely/python-sdk#123”

…with its test

msohailhussain

partial review

msohailhussain · 2020-07-16T19:37:40Z

optimizely/helpers/condition.py

@@ -32,6 +32,11 @@ class ConditionMatchTypes(object):
    GREATER_THAN = 'gt'
    LESS_THAN = 'lt'
    SUBSTRING = 'substring'
+    SEMVER_EQ = 'semver_eq'


Please alphabetize.

msohailhussain · 2020-07-16T19:38:58Z

optimizely/helpers/condition.py

+
+        return self.compare_user_version_with_target_version(index) <= 0
+
+    def semver_greater_than_and_equal_evaluator(self, index):


semver_greater_than_or_equal_evaluator

msohailhussain · 2020-07-16T19:39:11Z

optimizely/helpers/condition.py

+
+        return self.compare_user_version_with_target_version(index) == -1
+
+    def semver_less_than_and_equal_evaluator(self, index):


msohailhussain · 2020-07-16T19:39:28Z

optimizely/helpers/condition.py

    EVALUATORS_BY_MATCH_TYPE = {
        ConditionMatchTypes.EXACT: exact_evaluator,
        ConditionMatchTypes.EXISTS: exists_evaluator,
        ConditionMatchTypes.GREATER_THAN: greater_than_evaluator,
        ConditionMatchTypes.LESS_THAN: less_than_evaluator,
        ConditionMatchTypes.SUBSTRING: substring_evaluator,
+        ConditionMatchTypes.SEMVER_EQ: semver_equal_evaluator,


I will appreciate please alphabetize.

msohailhussain · 2020-07-16T19:39:59Z

optimizely/helpers/condition.py

+        user_version_parts = user_version.split(".")
+
+        for (idx, _) in enumerate(target_version_parts):
+            if len(user_version_parts) <= idx:


Take it variable outside if, so don't need to call len again and again.

msohailhussain · 2020-07-16T19:49:07Z

tests/base.py

@@ -837,13 +840,46 @@ def setUp(self, config_dict='config_dict'):
                        ],
                    ],
                },
+                {
+                    "id": "18278344267",
+                    "name": "semver_audience",


give proper audience name.

msohailhussain · 2020-07-16T19:49:25Z

tests/base.py

+                                {
+                                    "value": "1.2.3",
+                                    "type": "custom_attribute",
+                                    "name": "Android",


give proper attribute name.

msohailhussain · 2020-07-16T19:53:13Z

tests/helpers_tests/test_condition.py

@@ -24,6 +24,17 @@
 integerCondition = ['num_users', 10, 'custom_attribute', 'exact']
 doubleCondition = ['pi_value', 3.14, 'custom_attribute', 'exact']

+eq_semver_condition_list_variation_1 = [['Android', "2.0", 'custom_attribute', 'semver_eq']]


Give proper names, so anyone seeing testcase, should able to judge what this variable holds.

msohailhussain · 2020-07-16T19:57:59Z

tests/helpers_tests/test_condition.py

+
+        self.assertStrictTrue(evaluator.evaluate(0))
+
+    def test_evaluate__returns_true__when_user_version_does_not_match_target_version(self):


returns_true, interesting...
what's the difference between the following, both returning true.
test_evaluate__returns_true__when_user_version_does_not_match_target_version
test_evaluate__returns_true__when_user_version_matches_target_version

msohailhussain · 2020-07-16T19:58:58Z

tests/helpers_tests/test_condition.py

+
+        self.assertStrictFalse(evaluator.evaluate(0))
+
+    def test_evaluate__returns_true__when_user_version_is_gt_target_version(self):


Please rename all unit test method names names.

oakbani

Minor feedback.

oakbani · 2020-07-17T11:43:26Z

optimizely/helpers/condition.py

@@ -233,12 +238,74 @@ def substring_evaluator(self, index):

        return condition_value in user_value

+    def semver_equal_evaluator(self, index):


Please add doc string for all the new methods. See other method docs for reference.

oakbani · 2020-07-17T11:52:59Z

optimizely/helpers/condition.py

+                return -1
+            # compare strings e.g: n1.n2.n3-alpha/beta
+            if not user_version_parts[idx].isdigit():
+                if user_version_parts[idx] != target_version_parts[idx]:


my understanding here is that if string contains alpha/beta, we look for exact match. If this is the case, we should also exit with 0 here if it matches.

We are just copying logic from swift, so no worries. w

oakbani

Please address

oakbani · 2020-07-28T11:43:58Z

optimizely/helpers/condition.py

@@ -19,6 +19,8 @@
 from . import validator
 from .enums import CommonAudienceEvaluationLogs as audience_logs

+class OptimizelyErrors():
+    AttributeFormatInvalid = "Provided attributes are in an invalid format."


We normally add these constants to enums.py. Use an existing or add a new message in there. https://github.com/AmnaEjaz/python-sdk/blob/master/optimizely/helpers/enums.py#L96

oakbani · 2020-07-28T11:48:10Z

optimizely/helpers/condition.py

+        return "-" in target
+
+    def pre_release_separator(self):
+        return "-"


why not make this a constant instead?

I wanted to do that but i thought i am supposed to replicate the swift code as it is. Will make the suggested changes now

oakbani · 2020-07-28T11:48:31Z

optimizely/helpers/condition.py

+        return "+" in target
+
+    def build_separator(self):
+        return "+"


same. why not a constant?

oakbani · 2020-07-28T11:49:29Z

optimizely/helpers/condition.py

+            - True if the given version does contain "-"
+            - False if it doesn't
+    """
+        return "-" in target


use that constant here instead of hardcoding "-"

oakbani · 2020-07-28T11:52:38Z

optimizely/helpers/condition.py

+        target_suffix = ""
+
+        if self.is_pre_release(target) or self.is_build(target):
+            target_parts = target.split(self.pre_release_separator() if self.is_pre_release(target) else self.build_separator())


if (self.is_pre_release(target): split here elif (self.is_build()): split here

This will make the code more readable and you won't be calling is_pre_release twice.

This will improve readability but at the cost of repetition. For instance:

if self.is_pre_release((target)): target_parts = target.split(self.pre_release_separator()) if len(target_parts) < 1: raise Exception(Errors.INVALID_ATTRIBUTE_FORMAT) target_prefix = str(target_parts[0]) target_suffix = target_parts[1:] elif self.is_build(target): target_parts = target.split(self.build_separator()) if len(target_parts) < 1: raise Exception(Errors.INVALID_ATTRIBUTE_FORMAT) target_prefix = str(target_parts[0]) target_suffix = target_parts[1:]

You can move out all logic by

if target_parts: if len(target_parts) < 1: raise Exception(Errors.INVALID_ATTRIBUTE_FORMAT) target_prefix = str(target_parts[0]) target_suffix = target_parts[1:]

oakbani · 2020-07-28T11:53:42Z

optimizely/helpers/condition.py

+            target_version_parts.extend(target_suffix)
+            return target_version_parts
+        else:
+            return target_version_parts


when you are returning in both cases? Why not have a single return after if block ends.

mnoman09

Overall Looks good. Please resolve the comments.

mnoman09 · 2020-07-29T13:20:21Z

optimizely/helpers/condition.py

+        None:
+          - if the user version value is not string type or is null.
+    """
+        return self.compare_user_version_with_target_version(index) == 1


Suggested change

return self.compare_user_version_with_target_version(index) == 1

return self.compare_user_version_with_target_version(index) > 0

mnoman09 · 2020-07-29T13:20:45Z

optimizely/helpers/condition.py

+        None:
+          - if the user version value is not string type or is null.
+    """
+        return self.compare_user_version_with_target_version(index) == -1


Suggested change

return self.compare_user_version_with_target_version(index) == -1

return self.compare_user_version_with_target_version(index) < 0

mnoman09 · 2020-07-29T13:24:26Z

optimizely/helpers/condition.py

+        return self.compare_user_version_with_target_version(index) == 1
+
+    def semver_less_than_evaluator(self, index):
+        """ Evaluate the given semver less than match target version for the user version.


use "semantic version" instead of "semver" in all comments.

mnoman09 · 2020-07-29T13:33:54Z

optimizely/helpers/condition.py

+        target_suffix = ""
+
+        if self.is_pre_release(target) or self.is_build(target):
+            target_parts = target.split(SemverType.IS_PRE_RELEASE if self.is_pre_release(target) else SemverType.IS_BUILD)


What if the semantic version = "2.1.2-beta.2+2.3" will it split preRelease and Build number seperately? I think it will only split on preRelease ("-") in the given case.
This will cause the problem when comparing the preRelease version as we ignore the build version during comparison.
@msohailhussain can you please ask Thomas if the above explanation I gave is correct. I can add the test case in FSC as well.

mnoman09 · 2020-07-29T13:37:02Z

tests/helpers_tests/test_condition.py

+        )
+
+        self.assertStrictFalse(evaluator.evaluate(0))
+


nit: remove extra lines

mnoman09 · 2020-07-29T13:52:04Z

optimizely/helpers/condition.py

+                elif user_version_part < target_version_part:
+                    return -1
+        if user_version_parts_len > target_version_parts_len:
+            if self.is_patch_pre_release(user_version_parts_len-1, user_version_parts[user_version_parts_len-1]):


this is not needed.

mnoman09 · 2020-07-29T13:53:49Z

tests/base.py

            ],
            'groups': [],
            'attributes': [
                {'key': 'house', 'id': '594015'},
                {'key': 'lasers', 'id': '594016'},
                {'key': 'should_do_it', 'id': '594017'},
                {'key': 'favorite_ice_cream', 'id': '594018'},
+                {'key': 'android-release', 'id': '594019'},
+


nit: remove this line

mnoman09 · 2020-07-29T14:00:04Z

tests/helpers_tests/test_condition.py

+
+        self.assertStrictTrue(evaluator.evaluate(0))
+
+        evaluator = condition_helper.CustomAttributeConditionEvaluator(


Separate each case into separate definition. In case of failure It would be helpful to identify the exact failed scenario.

mnoman09 · 2020-07-29T14:01:17Z

tests/helpers_tests/test_condition.py

+
+        self.assertStrictFalse(evaluator.evaluate(0))
+
+        evaluator = condition_helper.CustomAttributeConditionEvaluator(


Same as above separate each individual case, with self explanatory name of def.

mnoman09 · 2020-07-29T14:09:27Z

tests/helpers_tests/test_condition.py

@@ -24,6 +24,20 @@
 integerCondition = ['num_users', 10, 'custom_attribute', 'exact']
 doubleCondition = ['pi_value', 3.14, 'custom_attribute', 'exact']



Update the header.

Amna Ejaz added 8 commits July 13, 2020 17:15

semver added - wip

686d6a3

added utility fn to compare functions

89e68e0

unit tests added for semver

80627b9

reverted unecessary changes

f9aec0a

Update test_condition.py

24b6088

ran flake8

379cb19

updated compare fn, added more unit tests also added semver_audience …

fb1cbc0

…with its test

updated as per instructions

26ad036

msohailhussain commented Jul 16, 2020

View reviewed changes

Amna Ejaz added 2 commits July 17, 2020 12:31

updated as per instructions

7e59b6f

Test cases added for no user version and null version

d840e00

oakbani requested changes Jul 17, 2020

View reviewed changes

Amna Ejaz added 4 commits July 18, 2020 13:36

Doc string added with semver methods

374e6d0

Updated python semver

6283910

Python semver updated with all test cases passing

8ea0941

added doc strings to methods

4382ad8

oakbani requested changes Jul 28, 2020

View reviewed changes

Amna Ejaz added 2 commits July 28, 2020 18:52

updated

8ec280b

semver updated

f560c9f

mnoman09 suggested changes Jul 29, 2020

View reviewed changes

Merge branch 'amna-master' into AmnaEjaz/semver-python

dbf42b7


		return self.compare_user_version_with_target_version(index) <= 0

		def semver_greater_than_and_equal_evaluator(self, index):


		return self.compare_user_version_with_target_version(index) == -1

		def semver_less_than_and_equal_evaluator(self, index):


		self.assertStrictTrue(evaluator.evaluate(0))

		def test_evaluate__returns_true__when_user_version_does_not_match_target_version(self):


		self.assertStrictFalse(evaluator.evaluate(0))

		def test_evaluate__returns_true__when_user_version_is_gt_target_version(self):

		@@ -233,12 +238,74 @@ def substring_evaluator(self, index):

		return condition_value in user_value

		def semver_equal_evaluator(self, index):


		self.assertStrictTrue(evaluator.evaluate(0))

		evaluator = condition_helper.CustomAttributeConditionEvaluator(

		@@ -24,6 +24,20 @@
		integerCondition = ['num_users', 10, 'custom_attribute', 'exact']
		doubleCondition = ['pi_value', 3.14, 'custom_attribute', 'exact']

Amna ejaz/semver python #1

Are you sure you want to change the base?

Amna ejaz/semver python #1

Uh oh!

Conversation

msohailhussain commented Jul 16, 2020

Summary

Test plan

Issues

Uh oh!

msohailhussain left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oakbani left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oakbani left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmnaEjaz Jul 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mnoman09 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmnaEjaz Jul 28, 2020 •

edited

Loading