-
Notifications
You must be signed in to change notification settings - Fork 30
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(OptimizelyConfig): add new fields to OptimizelyConfig #418
Changes from all commits
3587d11
f069a56
29b43c8
cdd3292
a1a9139
e6fa26d
8d5def4
f1e0e38
4b3ee37
ac627cd
c4e79d4
0bd4f25
e90fc30
d91c49c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -71,6 +71,40 @@ enum ConditionHolder: Codable, Equatable { | |||||||||||
return try conditions.evaluate(project: project, attributes: attributes) | ||||||||||||
} | ||||||||||||
} | ||||||||||||
|
||||||||||||
} | ||||||||||||
|
||||||||||||
// MARK: - serialization | ||||||||||||
|
||||||||||||
extension ConditionHolder { | ||||||||||||
|
||||||||||||
/// Returns a serialized string of audienceConditions | ||||||||||||
/// - each audienceId is converted into "AUDIENCE(audienceId)", which can be translated to correponding names later | ||||||||||||
/// | ||||||||||||
/// Examples: | ||||||||||||
/// - "123" => "AUDIENCE(123)" | ||||||||||||
/// - ["and", "123", "456"] => "AUDIENCE(123) AND AUDIENCE(456)" | ||||||||||||
/// - ["or", "123", ["and", "456", "789"]] => "AUDIENCE(123) OR ((AUDIENCE(456) AND AUDIENCE(789))" | ||||||||||||
var serialized: String { | ||||||||||||
switch self { | ||||||||||||
case .logicalOp: | ||||||||||||
return "" | ||||||||||||
case .leaf(.audienceId(let audienceId)): | ||||||||||||
return "AUDIENCE(\(audienceId))" | ||||||||||||
case .array(let conditions): | ||||||||||||
return "\(conditions.serialized)" | ||||||||||||
default: | ||||||||||||
return "" | ||||||||||||
} | ||||||||||||
} | ||||||||||||
|
||||||||||||
var isArray: Bool { | ||||||||||||
if case .array = self { | ||||||||||||
return true | ||||||||||||
} else { | ||||||||||||
return false | ||||||||||||
} | ||||||||||||
Comment on lines
+104
to
+106
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not important but i think this would look cleaner.
Suggested change
|
||||||||||||
} | ||||||||||||
} | ||||||||||||
|
||||||||||||
// MARK: - [ConditionHolder] | ||||||||||||
|
@@ -118,4 +152,40 @@ extension Array where Element == ConditionHolder { | |||||||||||
} | ||||||||||||
} | ||||||||||||
|
||||||||||||
/// Represents an array of ConditionHolder as a serialized string | ||||||||||||
/// | ||||||||||||
/// Examples: | ||||||||||||
/// - ["not", A] => "NOT A" | ||||||||||||
/// - ["and", A, B] => "A AND B" | ||||||||||||
/// - ["or", A, ["and", B, C]] => "A OR (B AND C)" | ||||||||||||
/// - [A] => "A" | ||||||||||||
var serialized: String { | ||||||||||||
var result = "" | ||||||||||||
|
||||||||||||
guard let firstItem = self.first else { | ||||||||||||
return "\(result)" | ||||||||||||
} | ||||||||||||
|
||||||||||||
// The first item of the array is supposed to be a logical op (and, or, not) | ||||||||||||
// extract it first and join the rest of the array items with the logical op | ||||||||||||
switch firstItem { | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we have comments here? |
||||||||||||
case .logicalOp(.not): | ||||||||||||
result = (self.count < 2) ? "" : "NOT \(self[1].serialized)" | ||||||||||||
case .logicalOp(let op): | ||||||||||||
result = self.enumerated() | ||||||||||||
.filter { $0.offset > 0 } | ||||||||||||
.map { | ||||||||||||
let desc = $0.element.serialized | ||||||||||||
return ($0.element.isArray) ? "(\(desc))" : desc | ||||||||||||
} | ||||||||||||
.joined(separator: " " + "\(op)".uppercased() + " ") | ||||||||||||
case .leaf(.audienceId): | ||||||||||||
result = "\([[ConditionHolder.logicalOp(.or)], self].flatMap({$0}).serialized)" | ||||||||||||
default: | ||||||||||||
result = "" | ||||||||||||
} | ||||||||||||
|
||||||||||||
return "\(result)" | ||||||||||||
} | ||||||||||||
|
||||||||||||
} |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -16,7 +16,7 @@ | |||
|
||||
import Foundation | ||||
|
||||
struct Experiment: Codable, Equatable { | ||||
struct Experiment: Codable, OptimizelyExperiment { | ||||
enum Status: String, Codable { | ||||
case running = "Running" | ||||
case launched = "Launched" | ||||
|
@@ -35,17 +35,29 @@ struct Experiment: Codable, Equatable { | |||
var audienceConditions: ConditionHolder? | ||||
// datafile spec defines this as [String: Any]. Supposed to be [ExperimentKey: VariationKey] | ||||
var forcedVariations: [String: String] | ||||
} | ||||
|
||||
enum CodingKeys: String, CodingKey { | ||||
case id, key, status, layerId, variations, trafficAllocation, audienceIds, audienceConditions, forcedVariations | ||||
} | ||||
|
||||
// MARK: - OptimizelyConfig | ||||
// MARK: - OptimizelyConfig | ||||
|
||||
extension Experiment: OptimizelyExperiment { | ||||
var variationsMap: [String: OptimizelyVariation] { | ||||
var map = [String: Variation]() | ||||
variations.forEach { | ||||
map[$0.key] = $0 | ||||
} | ||||
return map | ||||
var variationsMap: [String: OptimizelyVariation] = [:] | ||||
// replace with serialized string representation with audience names when ProjectConfig is ready | ||||
var audiences: String = "" | ||||
} | ||||
|
||||
extension Experiment: Equatable { | ||||
static func == (lhs: Experiment, rhs: Experiment) -> Bool { | ||||
return lhs.id == rhs.id && | ||||
lhs.key == rhs.key && | ||||
lhs.status == rhs.status && | ||||
lhs.layerId == rhs.layerId && | ||||
lhs.variations == rhs.variations && | ||||
lhs.trafficAllocation == rhs.trafficAllocation && | ||||
lhs.audienceIds == rhs.audienceIds && | ||||
lhs.audienceConditions == rhs.audienceConditions && | ||||
lhs.forcedVariations == rhs.forcedVariations | ||||
} | ||||
} | ||||
|
||||
|
@@ -63,4 +75,63 @@ extension Experiment { | |||
var isActivated: Bool { | ||||
return status == .running | ||||
} | ||||
|
||||
mutating func serializeAudiences(with audiencesMap: [String: String]) { | ||||
guard let conditions = audienceConditions else { return } | ||||
|
||||
let serialized = conditions.serialized | ||||
audiences = replaceAudienceIdsWithNames(string: serialized, audiencesMap: audiencesMap) | ||||
} | ||||
|
||||
/// Replace audience ids with audience names | ||||
/// | ||||
/// example: | ||||
/// - string: "(AUDIENCE(1) OR AUDIENCE(2)) AND AUDIENCE(3)" | ||||
/// - replaced: "(\"us\" OR \"female\") AND \"adult\"" | ||||
/// | ||||
/// - Parameter string: before replacement | ||||
/// - Returns: string after replacement | ||||
func replaceAudienceIdsWithNames(string: String, audiencesMap: [String: String]) -> String { | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we have unit test for this method. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have a unit test case covering this audience mapping -
|
||||
let beginWord = "AUDIENCE(" | ||||
let endWord = ")" | ||||
var keyIdx = 0 | ||||
var audienceId = "" | ||||
var collect = false | ||||
|
||||
var replaced = "" | ||||
for ch in string { | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we have some comments here for explanation? |
||||
// extract audience id in parenthesis (example: AUDIENCE("35") => "35") | ||||
if collect { | ||||
if String(ch) == endWord { | ||||
// output the extracted audienceId | ||||
replaced += "\"\(audiencesMap[audienceId] ?? audienceId)\"" | ||||
collect = false | ||||
audienceId = "" | ||||
} else { | ||||
audienceId += String(ch) | ||||
} | ||||
continue | ||||
} | ||||
|
||||
// walk-through until finding a matching keyword "AUDIENCE(" | ||||
if ch == Array(beginWord)[keyIdx] { | ||||
keyIdx += 1 | ||||
if keyIdx == beginWord.count { | ||||
keyIdx = 0 | ||||
collect = true | ||||
} | ||||
continue | ||||
} else { | ||||
if keyIdx > 0 { | ||||
replaced += Array(beginWord)[..<keyIdx] | ||||
} | ||||
keyIdx = 0 | ||||
} | ||||
|
||||
// pass through other characters | ||||
replaced += String(ch) | ||||
} | ||||
|
||||
return replaced | ||||
} | ||||
} |
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 have comments here to explain .leaf and .array cases? why these are required?