Allow methods + lambdas for on_error callbacks / middleware

[odlp]

Description

Recently I used add_on_error to add a callback, and incorrectly assumed I could use a Method interchangeably with a proc, e.g:

Bugsnag.configure do |config|
  config.add_on_error(Bugsnag::SomeCallback.method(:process))
end

This ended up breaking the middleware stack, because an exception was thrown. I missed the warnings emitted:

Bugsnag middleware error: undefined method `new' for #<Method:0x00007fdfc70f7b30>

The reason I'd like to use a class here is it's easier to unit test. The solution ended up being to wrap this in a proc:

Bugsnag.configure do |config|
  config.add_on_error(proc { |report| Bugsnag::SomeCallback.process(report) })
end

Describe the solution you'd like

Could middlewares be partitioned whether they respond_to?(:call)? This would provide middleware implementors the option of providing a proc, lambda or method.

Quick, incomplete example:

diff --git a/lib/bugsnag/middleware_stack.rb b/lib/bugsnag/middleware_stack.rb
index d9dcd95..b083d2f 100644
--- a/lib/bugsnag/middleware_stack.rb
+++ b/lib/bugsnag/middleware_stack.rb
@@ -131,8 +131,8 @@ module Bugsnag
     #
     # @return [Array<Proc>]
     def middleware_procs
-      # Split the middleware into separate lists of Procs and Classes
-      procs, classes = @middlewares.partition {|middleware| middleware.is_a?(Proc) }
+      # Split the middleware into separate lists of Callables (procs, lambdas, methods) and Classes
+      callables, classes = @middlewares.partition {|middleware| middleware.respond_to?(:call) }
 
       # Wrap the classes in a proc that, when called, news up the middleware and
       # passes the next middleware in the queue
@@ -140,12 +140,12 @@ module Bugsnag
         proc {|next_middleware| middleware.new(next_middleware) }
       end
 
-      # Wrap the list of procs in a proc that, when called, wraps them in an
+      # Wrap the list of callables in a proc that, when called, wraps them in an
       # 'OnErrorCallbacks' instance that also has a reference to the next middleware
-      wrapped_procs = proc {|next_middleware| OnErrorCallbacks.new(next_middleware, procs) }
+      wrapped_callables = proc {|next_middleware| OnErrorCallbacks.new(next_middleware, callables) }
 
       # Return the combined middleware and wrapped procs
-      middleware_instances.push(wrapped_procs)
+      middleware_instances.push(wrapped_callables)
     end
   end
 end

Following through, OnErrorCallbacks only expects a callback to respond to call:

bugsnag-ruby/lib/bugsnag/on_error_callbacks.rb

Line 14 in 98b73dc

should_continue = callback.call(report)

Describe alternatives you've considered

Raise an exception (in dev / test?) if anything other than a Proc or Class is passed as a middleware.

Additional context

Happy to contribute a PR if this is judged to be a worthwhile feature.

[yousif-bugsnag]

Hi @odlp, thanks for raising this!

We like the suggestion as it makes it a bit more ergonomic, and we'd certainly accept a PR if you'd like to submit one?

A couple of things we've noticed:

• Your current workaround could be made a bit nicer by calling to_proc on the Method:
  config.add_on_error(Bugsnag::SomeCallback.method(:process).to_proc)
• Lambdas should already be supported since they are Proc objects under the hood.

[odlp]

Your current workaround could be made a bit nicer by calling to_proc on the Method

Good point, thank you!

PR incoming with the respond_to?(:call) solution.

[yousif-bugsnag]

Closing this in favour of #662