feat(client): Allow user profile service and overrides to be passed in. #168
Conversation
| } | ||
|
|
||
| // OptionFunc is a type to a proper func | ||
| type OptionFunc func(*OptimizelyClient) | ||
| type OptionFunc func(*OptimizelyFactory) | ||
|
|
||
| // Client gets client and sets some parameters | ||
| func (f OptimizelyFactory) Client(clientOptions ...OptionFunc) (*OptimizelyClient, error) { |
There was a problem hiding this comment.
Nit, unrelated to your PR: is there a good reason that this method is defined with a value receiver, instead of a pointer receiver?
There was a problem hiding this comment.
Yeah because this method does not need to make changes to the OptimizelyFactory itself so it's safer just to make it a value receiver
| overrideStore ExperimentOverrideStore | ||
| userProfileService UserProfileService |
There was a problem hiding this comment.
Nit: it's a little weird that these fields are just use to move values from the option functions to a place where NewCompositeExperimentService can use them. Not sure there is a better alternative though.
There was a problem hiding this comment.
Yeah it was weird to me but at least they are private
pkg/decision/composite_service.go
Outdated
| if compositeService.compositeFeatureService == nil { | ||
| compositeService.compositeFeatureService = NewCompositeFeatureService(compositeService.compositeExperimentService) | ||
| } |
There was a problem hiding this comment.
Should we just assign this instead of check for nil, since there is not yet an option func that allows passing in compositeFeatureService?
There was a problem hiding this comment.
Functional options in go language are usually needed to dynamically initialize an object:
https://medium.com/@dreissenzahn/functional-options-in-go-35279f535c6
The exception is that our polling config manager is doing some heavy stuff during our initialization, that's why it can be a good candidate to check after the functional options. I still think that all the light default initialization has to take place before functional options like in the example above. That will eliminate a lot of if/else and make code cleaner.
|
@pawels-optimizely It seems more complex/potentially confusing to allow "some, but not all, fields are initialized prior to calling option functions", as opposed to initializing all fields before options, or none. |
There was a problem hiding this comment.
LGTM, but I'm open to additional suggestions/changes from @pawels-optimizely.
Summary
Tests