-
Notifications
You must be signed in to change notification settings - Fork 777
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
Possible rules prototype #4246
base: master
Are you sure you want to change the base?
Possible rules prototype #4246
Conversation
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.
Good approach. Recommend next steps are to flesh out the Rules engine tree structure with full path lookup and default support per the spec.
@@ -0,0 +1,53 @@ | |||
package optimizationmodule |
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.
The real location should be "modules\prebid\optimization" with the package name "optimization".
optimizationmodule/Function.go
Outdated
} | ||
|
||
// will be used in Build rules trie | ||
func NewFunction(name string, params []string) Function { |
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 should return an error, to allow the function builders to validate their input and return an error or warning.
optimizationmodule/Function.go
Outdated
|
||
// will be used in Build rules trie | ||
func NewFunction(name string, params []string) Function { | ||
switch name { |
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.
We'll want to make this case insensitive. Recommend converting to lowercase and having all the cases expressed in lowercase too.
optimizationmodule/Function.go
Outdated
case "setDeviceIP": | ||
return NewSetDevIp(params) | ||
default: | ||
return nil |
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 should be at least a warning, if a function name is not found / supported.
return "yes", nil | ||
} | ||
return "no", nil | ||
} |
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.
Looks good. We can make the yes / no constants to reuse in other functions... or should these be "true" and "false"?
|
||
func (sdip *SetDeviceIp) Call(rw *openrtb_ext.RequestWrapper) (string, error) { | ||
rw.Device.IP = sdip.IP | ||
return "", nil |
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 will need to use the modifier pattern. Instead of making the change right here, it needs to return a change set mutation.
type Node struct { | ||
Function Function | ||
// string because "rules": [{ "conditions": ["true", "false"], | ||
Children map[string]*Node |
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 the nodes be values instead of pointers?
IP: "0.0.0.0", | ||
}, | ||
}} | ||
err := rules.Execute(rw) |
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.
The params first need to be extracted, then given to Execute, and then Execute will use the params to locate a node. This will involve some backtracking behavior for defaults.
No description provided.