Remove ActiveSupport dependency #99
Conversation
… active support is not loaded
lib/circuitbox/circuit_breaker.rb
Outdated
| @@ -30,7 +30,7 @@ def initialize(service, options = {}) | |||
| @notifier = options.fetch(:notifier) { Circuitbox.default_notifier } | |||
|
|
|||
| @exceptions = options.fetch(:exceptions) { [] } | |||
| @exceptions = [Timeout::Error] if @exceptions.blank? | |||
| @exceptions = [Timeout::Error] if @exceptions.empty? | |||
There was a problem hiding this comment.
If there is not one already, I would add a test when @exceptions = nil to be sure the behavior is covered the same way by empty?
There was a problem hiding this comment.
If someone passes { exceptions: nil } empty is going to fail with NoMethodError (undefined method `empty?' for nil:NilClass). blank? would have returned true. I can't come up with a reason to use circuitbox and pass in no exceptions to monitor so I'm fine with leaving this how it is. Worst case we could do Array(@exceptions) but that just feels like allowing the API to be used incorrectly.
There was a problem hiding this comment.
I agree this would not be a "tell don't ask" situation here as the default on Timeout::Error is not obvious.
If we want to discourage bad API usage, I think we should then be explicit in the error.
raise 'exceptions need to be an array' unless @exceptions.is_a?(Array)There was a problem hiding this comment.
I think that makes sense. I'm wondering if we can do a little more cleanup (another PR) where we can move Timeout::Error to the options.fetch call otherwise if :exceptions are specified it needs to be given something that's not empty.
There were a few places around circuitbox where some active support methods were used, but there was not that many left. I switched those to use methods that come part of ruby. Also added a null notifier that is the default if active support notifications are not defined.