-
Notifications
You must be signed in to change notification settings - Fork 35
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
feat: get_optimizely_config API #226
Conversation
2 similar comments
optimizely/optimizely_config.py
Outdated
|
||
def __init__(self, project_config): | ||
""" | ||
Arguments: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit. Args.
optimizely/optimizely_config.py
Outdated
|
||
|
||
class OptimizelyVariable(object): | ||
def __init__(self, id, key, type, value): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type is a keyword and shadows a built in name. Would recommend naming this variable something else. May be variable_type
optimizely/optimizely_config.py
Outdated
Returns: | ||
Optimizely Config instance. | ||
""" | ||
self._create_lookup_maps() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems awkward that a get is calling a create.
optimizely/optimizely_config.py
Outdated
def _get_variables_map(self, variation, experiment): | ||
""" Gets variables map for given variation and experiment. | ||
|
||
Arguments: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Args and not Arguments. Apply this feedback throughout this PR.
optimizely/optimizely_config.py
Outdated
self.groups = project_config.groups | ||
self.revision = project_config.revision | ||
|
||
def get_optimizely_config(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit. Not sure if the naming has to be get_optimizely_config
, but OptimizelyConfigService.get_optimizely_config
has too much redundant information in it.
Something as simple as get_config
may suffice here. cc @jaeopt
opt_obj = optimizely.Optimizely(json.dumps(self.config_dict_with_features)) | ||
opt_config = opt_obj.get_optimizely_config() | ||
self.assertIsInstance(opt_config, optimizely_config.OptimizelyConfig) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add tests for OptConfig contents validation as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks great! I suggest to add more unit tests for OptConfig contents validation.
@jaeopt Have added unit tests. Please take a look. |
@@ -182,14 +182,12 @@ def setUp(self, config_dict='config_dict'): | |||
{ | |||
'id': '122239', | |||
'key': 'control', | |||
'featureEnabled': True, | |||
'variables': [{'id': '155551', 'value': '42.42'}], | |||
'variables': [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed these since this is not a feature experiment so shouldn't have these props.
class OptimizelyConfigService(object): | ||
""" Class encapsulating methods to be used in creating instance of OptimizelyConfig. """ | ||
|
||
def __init__(self, project_config): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to check instance
of ProjectConfig
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't expect to be used elsewhere. And in the main class, we validate project_config before using OptimizelyService. I can still validate if you so, but will have to keep a validity flag so that get_config returns nil and does not break.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my suggestion is to validate but @aliabbasrizvi will you suggest for validation here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with @msohailhussain, we should validate here.
optimizely/optimizely_config.py
Outdated
|
||
self.exp_id_to_feature_map = {} | ||
for feature in self.feature_flags: | ||
for id in feature['experimentIds']: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would instead of id
use experimentId
|
||
self.feature_key_variable_key_to_variable_map = {} | ||
self.feature_key_variable_id_to_variable_map = {} | ||
for feature in self.feature_flags: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
am unable to find the difference between this feature_flags
loop and at line 86. Can't we use one loop only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
optimizely/optimizely_config.py
Outdated
self._create_lookup_maps() | ||
|
||
def get_config(self): | ||
""" Returns instance of OptimizelyConfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks good. Comments could use some clarity.
optimizely/optimizely_config.py
Outdated
self.feature_key_variable_key_to_variable_map[feature['key']] = variables_key_map | ||
self.feature_key_variable_id_to_variable_map[feature['key']] = variables_id_map | ||
|
||
def _get_variables_map(self, variation, experiment): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit. I personally prefer arranging this as self, experiment, variation
to honor the parent child relationship between experiments and variations.
optimizely/optimizely_config.py
Outdated
""" Gets variables map for given variation and experiment. | ||
|
||
Args: | ||
variation dict |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very vague. Dict consisting of what?
@mikeng13 This is approved and i dont have rights to merge. Can you help. |
Summary
Adds support for get_optimizely_config public API.
Test plan
Issues