Dynamically register events to dispatch

[sambostock]

Note

This draft PR is a companion to Shopify/ruby-lsp#3291, which proposes this approach. Tests and other things have been omitted for now in the interest of getting feedback on the approach first, and will be added if we decide to move forward.

Instead of requiring the consumer to provide a list of all events which they wish to handle, we can give them to option of dynamically detecting them, by scanning the listener's public methods.

This approach is similar to that used by Minitest (scanning for test_ methods) and Rails generators (running all public methods in the order they are defined).

While this is slower than specifying a hard coded list, the penalty is only during registration. There is no change the the behaviour of dispatching the events. It is also a non-breaking change, so consumers can continue specifying each event if desired.

[kddnewton]

I would rather have a separate method for this, as this behavior gets magically triggered by having no events in the list currently. Something like:

def register(listener, *events)
  events.each { |event| (listeners[event] ||= []) << listener }
end

def register_public_methods(listener)
  register(listener, *listener.public_methods(false).grep(/^on_.*_(enter|leave)$/))
end

The other difference I would like to see I have in the example here, which is that I think we should not include inherited methods, (so using public_methods(false)).

[kddnewton]

I like this approach so I'm marking it ready for review. I also force-pushed to your branch the changes I suggested, so it should be good to go. Thanks for doing this!

[sambostock]

Thanks @kddnewton!

[sambostock]

Actually, @kddnewton under what circumstances would we not want to register inherited handlers? I can't think of a scenario in which that would be either introducing a bug or making things harder for the consumer, but maybe I'm missing something.

For example, if a consumer had some common functionality extracted into a parent class or module, this could break registration.

module HandleClassAndModuleNodeEnter
  def on_class_node_enter(node)
    on_class_or_module_enter(node)
  end

  def on_module_node_enter(node)
    on_class_or_module_enter(node)
  end
end

class MyListener
  include HandleClassAndModuleNodeEnter
  
  def initialize(dispatcher)
    dispatcher.register_public_methods(self)
  end

  def on_class_or_module_enter(node)
    # ...
  end
end

[kddnewton]

Yeah it was more a way to avoid mishandling things. On the off chance something snuck in through some unwitting module, it wouldn't get accidentally triggered.