-
Notifications
You must be signed in to change notification settings - Fork 36
feat (audience match types): Update audience evaluator and project config for new audience match types #147
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
Conversation
… oakbani/audience-match-types-project-config
optimizely/helpers/condition.py
Outdated
@@ -174,6 +174,10 @@ def is_finite(self, value): | |||
# numbers.Integral instead of int to accomodate long integer in python 2 | |||
return False | |||
|
|||
if isinstance(value, bool): |
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.
Doesn't this belong in #146?
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.
Yeah, but this showed up only when I as able to run new audiences compat suite tests with this PR. Will move it into #146.
tests/base.py
Outdated
{ | ||
'id': '3988293898', | ||
'name': 'substringString', | ||
'conditions': '["and", ["or", ["or", ' |
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.
After recent discussions, conditions
will not be stringified but instead object (or list of objects and boolean operators)
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.
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 see. Let me revise it.
… oakbani/audience-match-types-project-config
… oakbani/audience-match-types-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.
I still need to review #146 which this is based on, but this looks pretty good!
# Conditions of audiences in typedAudiences are not expected | ||
# to be string-encoded as they are in audiences. | ||
for typed_audience in self.typed_audiences: | ||
typed_audience['conditions'] = json.dumps(typed_audience['conditions']) |
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's kinda weird that we're calling json.dumps
on typedAudience
s instead of just calling json.loads
on audience
s, but it looks like ConditionDecoder
is the wrench in the works. Hopefully we can reconsider something like #145 in the future!
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: might be clearer to do this during the initial assignment to self.typedAudiences
(currently on line 50) instead of mutating self.typedAudiences
later. Not a big deal though
… oakbani/audience-match-types-project-config
… oakbani/audience-match-types-project-config
… oakbani/audience-match-types-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.
LGTM
88159da
into
oakbani/audience-match-type-condition-evaluator-2
Summary
This updates project config and the audience evaluator to finish the implementation of typed audience evaluation:
false
when no attributes were passed)Test Plan