feat: Optimizely factory added #325

ozayr-zaviar · 2021-05-06T12:31:47Z

Summary

optimizely_factory.py file added containing OptimizelyFactory class.
test_optimizely_factory.py file added in tests/ for test cases.

Test plan

All existed unit tests and FSC test should pass.
All unit tests should pass.

coveralls · 2021-05-06T12:37:15Z

Coverage increased (+0.03%) to 95.871% when pulling 5c7ff3e on uzair/static-polling-interval into 7607cf4 on master.

msohailhussain · 2021-05-07T19:00:30Z

optimizely/config_manager.py

@@ -196,7 +196,7 @@ def __init__(
        self.datafile_url = self.get_datafile_url(
            sdk_key, url, url_template or self.DATAFILE_URL_TEMPLATE
        )
-        self.set_update_interval(update_interval)


you are not supposed to change method name.

msohailhussain · 2021-05-07T19:09:48Z

optimizely/config_manager.py

        """ Helper method to set frequency at which datafile has to be polled and ProjectConfig updated.

        Args:
            update_interval: Time in seconds after which to update datafile.
        """
+
+        logger = optimizely_logger.adapt_logger(optimizely_logger.NoOpLogger())


no need to assign new logger.

msohailhussain · 2021-05-07T19:10:23Z

optimizely/config_manager.py

        if update_interval is None:
            update_interval = enums.ConfigManager.DEFAULT_UPDATE_INTERVAL
-            self.logger.debug('Setting config update interval to default value {}.'.format(update_interval))
+            logger.debug('Setting config update interval to default value {}.'.format(update_interval))


revert this line.

This reverts commit e696f1c.

mnoman09 · 2021-05-10T19:29:54Z

optimizely/optimizely_factory.py

@@ -0,0 +1,180 @@
+# Copyright 2016-2021, Optimizely


change the header year to 2021 only as this file is created in this year

mnoman09 · 2021-05-10T19:52:32Z

optimizely/optimizely_factory.py

+         """
+
+        if isinstance(flush_interval, bool):
+            logger.error('Flush interval is invalid, setting to default flush interval # {0}'.format(


avoid logging and validation here as there should be logging and validation already done somewhere in SDK.

This is fine to have some major validation here.

msohailhussain · 2021-05-12T17:16:47Z

optimizely/optimizely_factory.py

+          logger: Optional LoggerInterface Provides a log method to log messages.
+         """
+
+        if isinstance(batch_size, bool):


we don't need this condition.

msohailhussain · 2021-05-12T17:17:33Z

optimizely/optimizely_factory.py

+                BatchEventProcessor._DEFAULT_BATCH_SIZE))
+            return
+
+        if not batch_size > 0:


can't we use else or else if

msohailhussain · 2021-05-12T17:17:50Z

optimizely/optimizely_factory.py

+        return OptimizelyFactory.max_event_batch_size
+
+    @staticmethod
+    def set_flush_interval(flush_interval, logger=optimizely_logger.NoOpLogger().logger):


same as above comments.

msohailhussain · 2021-05-12T17:20:05Z

tests/test_optimizely_factory.py

+        optimizely_instance = OptimizelyFactory.default_instance('sdk_key')
+        self.assertIsInstance(optimizely_instance.config_manager, PollingConfigManager)
+
+    def test_default_instance_should_create_config_manager_when_polling_interval_and_blocking_timeout_are_set_valid(


shorten the name of func

jaeopt

A few suggestions and clarifications

jaeopt · 2021-05-13T18:35:34Z

optimizely/optimizely_factory.py

+    @staticmethod
+    def default_instance_with_config_manager(config_manager):
+        return Optimizely(
+            None, None, None, None, None, None, None, config_manager


Suggested change

None, None, None, None, None, None, None, config_manager

config_manager=config_manager

jaeopt · 2021-05-13T18:41:41Z

tests/test_optimizely_factory.py

+        # validate through validator
+        self.assertTrue(validator.is_config_manager_valid(CustomConfigManager()))


what's the point of validating one defined for test?

jaeopt · 2021-05-13T20:44:07Z

optimizely/optimizely_factory.py

+    blocking_timeout = None
+
+    @staticmethod
+    def set_batch_size(batch_size, logger=optimizely_logger.NoOpLogger().logger):


passing a logger to set a parameter sounds weird. Can't remove all these validation with logging here since those parameters are validated again in event_processor.

jaeopt · 2021-05-13T20:54:22Z

optimizely/optimizely_factory.py

+        config_manager = PollingConfigManager(**config_manager_options)
+
+        optimizely = Optimizely(
+            datafile, None, logger, error_handler, None, None, sdk_key, config_manager, notification_center


Shouldn't we include "BatchEventProcessor" as a part of this factory method as a common configuration?

Mat001 · 2021-05-13T21:05:26Z

optimizely/optimizely_factory.py

+    def set_batch_size(batch_size, logger=optimizely_logger.NoOpLogger().logger):
+        """ Convenience method for setting the maximum number of events contained within a batch.
+        Args:
+          batch_size: Sets size of EventQueue.


Change to lower case: event_queue.
I couldn't find class EentQueue in the sdk. Unless I missed smth

Mat001 · 2021-05-13T21:07:22Z

optimizely/optimizely_factory.py

+        """ Convenience method for setting the maximum number of events contained within a batch.
+        Args:
+          batch_size: Sets size of EventQueue.
+          logger: Optional LoggerInterface Provides a log method to log messages.


Same here: ----> loger interface.
Otherwise it may hint to a class which doesn't exist

Also lover case "provides"

Mat001 · 2021-05-13T21:13:22Z

optimizely/optimizely_factory.py

+        """ Convenience method for setting the maximum time interval in milliseconds between event dispatches.
+        Args:
+          flush_interval: Time interval between event dispatches.
+          logger: Optional LoggerInterface Provides a log method to log messages.


Use lower case "logging interface", and lower case "provides"

Mat001 · 2021-05-13T21:14:58Z

optimizely/optimizely_factory.py

+    def default_instance(sdk_key, datafile=None):
+        """ Returns a new optimizely instance..
+          Args:
+            sdk_key:  Required String uniquely identifying the fallback datafile corresponding to project.


Lower case "string". Python doesn't have static typing

Mat001 · 2021-05-13T21:16:29Z

optimizely/optimizely_factory.py

+
+        """ Returns a new optimizely instance.
+             if max_event_batch_size and max_event_flush_interval are None then default batch_size and flush_interval
+             will be used to setup batchEventProcessor.


change to "BatchEventProcessor"

Mat001 · 2021-05-13T21:18:45Z

optimizely/optimizely_factory.py

+                       By default nothing would be logged.
+               error_handler: Optional ErrorHandlerInterface which provides a handle_error method to handle exceptions.
+                              By default all exceptions will be suppressed.
+               skip_json_validation: Optional Boolean param to skip JSON schema validation of the provided datafile.


lower case boolean

Mat001 · 2021-05-13T21:19:27Z

optimizely/optimizely_factory.py

+               error_handler: Optional ErrorHandlerInterface which provides a handle_error method to handle exceptions.
+                              By default all exceptions will be suppressed.
+               skip_json_validation: Optional Boolean param to skip JSON schema validation of the provided datafile.
+               user_profile_service: Optional UserProfileServiceInterface Provides methods to store and retrieve


UserProfileService interface
provides

Mat001 · 2021-05-13T21:21:04Z

optimizely/optimizely_factory.py

+               skip_json_validation: Optional Boolean param to skip JSON schema validation of the provided datafile.
+               user_profile_service: Optional UserProfileServiceInterface Provides methods to store and retrieve
+                                     user profiles.
+               config_manager: Optional ConfigManagerInterface Responds to 'config' method.


ConfigManager interface
responds

Mat001 · 2021-05-13T21:26:02Z

tests/test_optimizely_factory.py

+        self.event_dispatcher = EventDispatcher()
+        self.user_profile_service = UserProfileService()
+
+    def test_default_instance_should_create_config_manager_when_sdk_key_is_given(self, _):


please use double underscore in test name after the main part of the test name. In All tests. For example:

test_default_instance__should_create_config_manager_when_sdk_key_is_given

test_default_instance__should_create_config_set_default_values_when_polling_int_and_blocking_time_invalid

etc

This is for consistency with other test names. Double underscore clarifies the specifics of the test in the second part of the test name, so it's nice to have that double underscore mark.

Mat001 · 2021-05-13T21:26:32Z

tests/test_optimizely_factory.py

+        self.assertEqual(optimizely_instance.config_manager.update_interval, 40)
+        self.assertEqual(optimizely_instance.config_manager.blocking_timeout, 5)
+
+    def test_default_instance_should_create_config_set_default_values_when_polling_int_and_blocking_time_invalid(


double underscore test_default_instance__...

jaeopt

LGTM

msohailhussain

lgtm

Mat001

lgtm

no review needed.

* polling interval updated * Revert "polling interval updated" This reverts commit e696f1c. * Create optimizely_factory.py * linting fixed * whitespaced removed * comments addressed and unit tests added * comments addressed * comments addressed

polling interval updated

e696f1c

optimizely-codereview-bot assigned ozayr-zaviar May 6, 2021

ozayr-zaviar removed their assignment May 6, 2021

msohailhussain reviewed May 7, 2021

View reviewed changes

ozayr-zaviar added 2 commits May 10, 2021 19:55

Revert "polling interval updated"

d4069df

This reverts commit e696f1c.

Create optimizely_factory.py

dba6442

ozayr-zaviar changed the title ~~refactor: polling interval function conversion to static function~~ feat: Optimizely factory added May 10, 2021

ozayr-zaviar and others added 3 commits May 10, 2021 20:42

linting fixed

12a94f9

Merge branch 'master' into uzair/static-polling-interval

e95a268

whitespaced removed

1e8ef28

mnoman09 previously requested changes May 10, 2021

View reviewed changes

comments addressed and unit tests added

8cd4ad0

msohailhussain reviewed May 12, 2021

View reviewed changes

msohailhussain marked this pull request as ready for review May 12, 2021 17:21

msohailhussain requested a review from a team as a code owner May 12, 2021 17:21

optimizely-codereview-bot added the 🤖 24 hrs label May 12, 2021

jaeopt requested changes May 13, 2021

View reviewed changes

optimizely-codereview-bot removed the 🤖 24 hrs label May 13, 2021

optimizely-codereview-bot assigned ozayr-zaviar May 13, 2021

Mat001 requested changes May 13, 2021

View reviewed changes

ozayr-zaviar added 2 commits May 17, 2021 14:21

comments addressed

ab4ea05

comments addressed

5e13d7b

msohailhussain requested review from jaeopt and mnoman09 May 24, 2021 05:37

msohailhussain self-requested a review May 24, 2021 05:37

msohailhussain unassigned ozayr-zaviar May 24, 2021

optimizely-codereview-bot requested a review from Mat001 May 24, 2021 05:37

optimizely-codereview-bot added the 🤖 4 hrs label May 24, 2021

jaeopt approved these changes May 25, 2021

View reviewed changes

optimizely-codereview-bot added 🤖 8 hrs 🤖 12 hrs and removed 🤖 4 hrs 🤖 8 hrs labels May 25, 2021

msohailhussain approved these changes May 25, 2021

View reviewed changes

yasirfolio3 force-pushed the uzair/static-polling-interval branch from 5ab1aa1 to 5e13d7b Compare May 26, 2021 09:13

optimizely-codereview-bot added 🤖 16 hrs and removed 🤖 12 hrs labels May 26, 2021

Mat001 approved these changes May 26, 2021

View reviewed changes

optimizely-codereview-bot added 🤖 20 hrs 🤖 24 hrs and removed 🤖 16 hrs 🤖 20 hrs labels May 26, 2021

Merge branch 'master' into uzair/static-polling-interval

5c7ff3e

msohailhussain merged commit cdc652e into master Jun 4, 2021

msohailhussain deleted the uzair/static-polling-interval branch June 4, 2021 05:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Optimizely factory added #325

feat: Optimizely factory added #325

ozayr-zaviar commented May 6, 2021 •

edited

Loading

coveralls commented May 6, 2021 •

edited

Loading

msohailhussain May 7, 2021

msohailhussain May 7, 2021

msohailhussain May 7, 2021

mnoman09 May 10, 2021

mnoman09 May 10, 2021

msohailhussain May 12, 2021

msohailhussain May 12, 2021

msohailhussain May 12, 2021

msohailhussain May 12, 2021

msohailhussain May 12, 2021

jaeopt left a comment

jaeopt May 13, 2021

jaeopt May 13, 2021

jaeopt May 13, 2021

jaeopt May 13, 2021

Mat001 May 13, 2021

Mat001 May 13, 2021

Mat001 May 13, 2021

Mat001 May 13, 2021

Mat001 May 13, 2021

Mat001 May 13, 2021

Mat001 May 13, 2021

Mat001 May 13, 2021

Mat001 May 13, 2021

Mat001 May 13, 2021

jaeopt left a comment

msohailhussain left a comment

Mat001 left a comment

	None, None, None, None, None, None, None, config_manager
	config_manager=config_manager

		# validate through validator
		self.assertTrue(validator.is_config_manager_valid(CustomConfigManager()))

feat: Optimizely factory added #325

feat: Optimizely factory added #325

Conversation

ozayr-zaviar commented May 6, 2021 • edited Loading

Summary

Test plan

coveralls commented May 6, 2021 • edited Loading

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

jaeopt 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

Choose a reason for hiding this comment

jaeopt left a comment

Choose a reason for hiding this comment

msohailhussain left a comment

Choose a reason for hiding this comment

Mat001 left a comment

Choose a reason for hiding this comment

ozayr-zaviar commented May 6, 2021 •

edited

Loading

coveralls commented May 6, 2021 •

edited

Loading