fix(blocking-api): fixed issue related to default blocking-timeout and blocking API methods. #202
Conversation
msohailhussain changed the title 
| ) | ||
| end | ||
| | ||
| def get_config |
There was a problem hiding this comment.
I think, this should be defined in the projectconfigmanager as an abstract method. right now i can see config is defined in the projectconfig manager.
msohailhussain changed the title msohailhussain changed the title 
| # we simply return config. | ||
| # If it is not, we wait and block maximum for @blocking_timeout. | ||
| # If thread is not running, we fetch the datafile and update config. | ||
| return nil if @closed == true |
There was a problem hiding this comment.
Why do we have to return nil in this case? Is there a problem with just returning the current @config? Also, why @closed == true instead of @closed?
| "Blocking timeout is not provided. Defaulting to #{Helpers::Constants::CONFIG_MANAGER['DEFAULT_BLOCKING_TIMEOUT']} seconds." | ||
| ) | ||
| @polling_interval = Helpers::Constants::CONFIG_MANAGER['DEFAULT_BLOCKING_TIMEOUT'] | ||
| @blocking_timeout = Helpers::Constants::CONFIG_MANAGER['DEFAULT_BLOCKING_TIMEOUT'] |
There was a problem hiding this comment.
Please ensure that there is test coverage that would have revealed these places where it should have been @blocking_timeout, but was actually @polling_interval.
| end | ||
| | ||
| def get_config | ||
| def config |
There was a problem hiding this comment.
why you are defining config not get_config, unable to understand.
There was a problem hiding this comment.
Following ruby's convention.
Please view rubocop's rule https://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Naming/AccessorMethodName
There was a problem hiding this comment.
I stumpled upon this, too. Not even your own StaticProjectConfigManager is using get_config which makes it unusable here:
Lines 85 to 88 in 31e93c7
| @msohailhussain Could you please review and approve if this looks good to you? |
| # we simply return config. | ||
| # If it is not, we wait and block maximum for @blocking_timeout. | ||
| # If thread is not running, we fetch the datafile and update config. | ||
| return @config if @closed |
There was a problem hiding this comment.
@msohailhussain I agree that stopped is a better attribute name.
README.md Outdated
| When the `datafile` is given then it will be used initially before any update. | ||
| 2. Initialize Optimizely by providing a Config Manager that implements a 'get_config' method. You can customize our `HTTPConfigManager` as needed. | ||
| 2. Initialize Optimizely by providing a Config Manager that implements a 'config' method. You can customize our `HTTPConfigManager` as needed. |
lib/optimizely/helpers/constants.rb Outdated
| CONFIG_MANAGER = { | ||
| 'DATAFILE_URL_TEMPLATE' => 'https://cdn.optimizely.com/datafiles/%s.json', | ||
| # Default time in seconds to block the get_config call until config has been initialized. | ||
| # Default time in seconds to block the config method call until config has been initialized. |
lib/optimizely/optimizely_factory.rb Outdated
| # Returns a new optimizely instance. | ||
| # | ||
| # @param config_manager - Required ConfigManagerInterface Responds to get_config. | ||
| # @param config_manager - Required ConfigManagerInterface Responds to config method. |
There was a problem hiding this comment.
please single quote. am saying config should be more focused as method name.
5 participants
Summary
blocking_timeoutis set to default.blocking_timeoutperiod or datafile is not loaded successfulsy.