feat: implementing OptimizelyJSON #250
Conversation
pawels-optimizely
requested review from
jaeopt,
mikecdavis,
mjc1283 and
msohailhussain
| "field1": 1, | ||
| "field2": 2.5, | ||
| "field3": "three", | ||
| "field4": map[string]interface{}{"inner_field1": 3, "inner_field2": suite.dynamicList}, |
There was a problem hiding this comment.
Can you please add a test case, how I can access dynamicList's element.
Ideally it should be field4.inner_field2[2] but I don't see index implementation for GetValue method.
There was a problem hiding this comment.
Sure, I will provide that test, although officially we won't support array object, but that does not prevent the user to include array in json .
mikecdavis
left a comment
mikecdavis
left a comment
There was a problem hiding this comment.
I agree with the previous comments made, I also suggested an optimization to store the string from the datafile and lazily unmarshal.
| func NewOptimizelyJSON(jsonRepr map[string]interface{}) *OptimizelyJSON { | ||
| return &OptimizelyJSON{jsonRepr: jsonRepr} | ||
| } |
There was a problem hiding this comment.
We haven't tied in the project config yet, but I think this should probably be instantiated with payload string since that is what we are storing in the datafile. Then, depending on the method calls below we'd, memoize the transformations into map[string]interface{} if that was ever requested. As it's written now I feel we'd be marshaling data back and forth unnecessarily.
There was a problem hiding this comment.
We haven't tied in the project config yet, but I think this should probably be instantiated with
payload stringsince that is what we are storing in the datafile. Then, depending on the method calls below we'd, memoize the transformations intomap[string]interface{}if that was ever requested. As it's written now I feel we'd be marshaling data back and forth unnecessarily.
I thought about that too, but then I thought that 2 out of 3 methods need map[string]interface{}, and only one method needs it as a string. But I agree that public SDK's would need to take the string and Unmarshal into that interface.
There was a problem hiding this comment.
I have implemented string payload, with map[string]interface{} used for memoization. I did look at the reflect package and wanted to avoid this memoization. Things get really complicated when the user does not provide a json tag. Unmarshal() can still match by field name, case insensitive. That would require creating a map of field names or something like that (steal some logic from Unmarshal()...), which I don't think will be that pretty if you want to preserve that functionality. Also, I don't think we can enforce the user to provide json tags all the time.
| } | ||
|
|
||
| // GetValue populates the schema passed by the user - it takes primitive types and complex struct type | ||
| func (optlyJson *OptimizelyJSON) GetValue(jsonPath string, schema interface{}) error { |
There was a problem hiding this comment.
I am still not sure about it's name. It should have better name. since it's not returning anything.
may be UnMarshal would be a better name.
There was a problem hiding this comment.
it's a suggestion, shouldn't block anything.
Summary