Skip to content

Conversation

pawels-optimizely
Copy link
Contributor

No description provided.

Copy link
Contributor

@mikeproeng37 mikeproeng37 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly good, I have a few suggestions!

@@ -26,14 +26,15 @@ type Audience struct {
// Experiment represents an Experiment object from the Optimizely datafile
type Experiment struct {
// @TODO(mng): include audienceConditions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can remove this comment from me 😄

switch v := node.Item.(type) {
case entities.Condition:
evaluator := CustomAttributeConditionEvaluator{}
result, err = evaluator.Evaluate(node.Item, condTreeParams)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are already special casing here, why not make the evaluator.Evaluate( ) take in the typed Condition instead of an interface. And the same for the AudienceConditionEvaluator since we already know that the item we are evaluating is a string?

if !evalResult {
// user not targeted for experiment, return an empty variation
experimentDecision.DecisionMade = true
experimentDecision.Variation = entities.Variation{}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line isn't needed right since it'll just be the default zero-value Variation

Copy link
Contributor

@mikeproeng37 mikeproeng37 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

# Conflicts:
#	optimizely/decision/experiment_targeting_service.go
@pawels-optimizely pawels-optimizely merged commit 896f726 into go-alpha Jul 15, 2019
@mikeproeng37 mikeproeng37 deleted the pawel/OASIS-4943 branch September 9, 2019 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants