refact: Updates OptimizelyFactory methods. #183
Conversation
lib/optimizely/optimizely_factory.rb
Outdated
| # Returns a new optimizely instance. | ||
| # | ||
| # @param config_manager - Required ConfigManagerInterface Responds to get_config. | ||
| def self.default_instance_with_manager(config_manager) |
lib/optimizely/optimizely_factory.rb
Outdated
| # @param user_profile_service - Optional UserProfileServiceInterface which provides methods to store and retreive user profiles. | ||
| # @param skip_json_validation - Optional Boolean param to skip JSON schema validation of the provided datafile. | ||
| # @param notification_center - Optional Instance of NotificationCenter. | ||
| def self.custom_instance_with_manager( |
There was a problem hiding this comment.
Maybe this is not needed. Let's keep it out until a customer asks for it. I don't see too much value in supporting this.
lib/optimizely/optimizely_factory.rb
Outdated
| event_dispatcher = nil, | ||
| logger = nil, | ||
| error_handler = nil, | ||
| skip_json_validation = false, | ||
| user_profile_service = nil, | ||
| sdk_key = nil, | ||
| config_manager = nil, |
There was a problem hiding this comment.
We should keep this config manager and pass it along to the constructor. We just shouldn't make it mandatory, default it to nil
lib/optimizely/optimizely_factory.rb
Outdated
| # @param notification_center - Optional Instance of NotificationCenter. | ||
| def self.custom_instance( | ||
| sdk_key, | ||
| fallback = nil, |
There was a problem hiding this comment.
Externally, this should still be called datafile. Internally, we can call it fallback.
There was a problem hiding this comment.
fallback is coming from java-sdk? do u still want to change it?
https://github.com/optimizely/java-sdk/blob/master/core-httpclient-impl/src/main/java/com/optimizely/ab/OptimizelyFactory.java#L152
There was a problem hiding this comment.
Hmm, yeah this is a named parameter so I think we should just call it datafile and in the doc param we can call it the fallback datafile. I don't like the idea of introducing yet another name/concept for it.
Summary