(feat): add a track call #72
Conversation
mikeproeng37
left a comment
mikeproeng37
left a comment
There was a problem hiding this comment.
Code looks good. Can you please add unit tests for it and also rename the PR to feat from chore since this is adding a new feature?
optimizely/client/client.go
Outdated
| userEvent := event.CreateConversionUserEvent(o.configManager.GetConfig(), configEvent, userContext, eventTags) | ||
| o.eventProcessor.ProcessEvent(userEvent) | ||
| } else { | ||
| logger.Error("Error getting event", err) |
There was a problem hiding this comment.
Please include the event key in the message
|
|
||
| if err == nil { | ||
| userEvent := event.CreateConversionUserEvent(o.configManager.GetConfig(), configEvent, userContext, eventTags) | ||
| o.eventProcessor.ProcessEvent(userEvent) |
thomaszurkan-optimizely
changed the title
thomaszurkan-optimizely
changed the title
optimizely/client/client.go
Outdated
| if !o.isValid { | ||
| errorMessage := "Optimizely instance is not valid. Failing GetEnabledFeatures." | ||
| err := errors.New(errorMessage) | ||
| logger.Error(errorMessage, nil) |
There was a problem hiding this comment.
logger.Error takes nil ?
something like this could be better:
err = errors.New("Optimizely instance is not valid. Failing GetEnabledFeatures.")
logger.Error("",err) or logger.Error(err.String(), err) // kind of redundant
return err
optimizely/client/client.go
Outdated
| defer func() { | ||
| if r := recover(); r != nil { | ||
| errorMessage := fmt.Sprintf(`Optimizely SDK is panicking with the error "%s"`, string(debug.Stack())) | ||
| err := errors.New(errorMessage) |
There was a problem hiding this comment.
do you want to return that err ? or just created for logger ?
There was a problem hiding this comment.
it's in a defer
There was a problem hiding this comment.
it's not going to change the value of the error returned that's why I am asking
There was a problem hiding this comment.
Ah he's right @thomaszurkan-optimizely, you need to name the variable in the return part of the function signature to (err error) so that you can assign err = errors.New(errorMessage instead of using :=
mikeproeng37
left a comment
mikeproeng37
left a comment
There was a problem hiding this comment.
See message on error message
No description provided.