Remove ActiveSupport::Notifications class methods? #5526

Closed
technoweenie opened this Issue Mar 20, 2012 · 5 comments

Comments

Projects
None yet
3 participants
Contributor

technoweenie commented Mar 20, 2012

Any interest in replacing these AS::Notifications class methods with some class?

module ActiveSupport::Notifications
  class Service
    def initialize(notifier)
      @notifier = notifier
      @instrumenter = Instrumenter.new(notifier)
    end

    def publish(name, *args)
      @notifier.publish name, *args
    end

    #ETC
  end
end

We can keep backwards compatibility:

module ActiveSupport::Notifications
  class << self
    attr_accessor :notifier, :service

    def publish(name, *args)
      service.publish name, *args
    end

    def service
      Thread.current[:"service_#{notifier.object_id}"] ||= Service.new(notifier)
    end
  end
end

This would give us some flexibility. We can have multiple notification "services" running, with different notifiers and subscribers. It looks like AS::LogSubscriber already assumes this is possible.

I'm happy to do the work, I just wanted to see if anyone hated the idea.

Owner

jeremy commented Mar 20, 2012

Sure - that'd be a fine refactoring!

Contributor

technoweenie commented Mar 21, 2012

Cool. I did the work here, but want to see how it works in production first. I'll bring it over in a pull request soon.

What about extracting some of the common LogSubscriber bits out? Specifically the #attach_to and #call methods. I like this pattern a lot and am thinking of doing something like a StatsdSubscriber.

Owner

jeremy commented Mar 22, 2012

Totally! We have a similar case: a bunch of code blobs floating in #subscribe blocks. Attaching a subscriber would really clean it up.

Member

drogus commented May 11, 2012

@technoweenie have you had time to work on that? As this issue is only a question on whether to not go on with such refactoring can I close it? You could then submit a new pull request when you're ready with the code.

Contributor

technoweenie commented May 11, 2012

Go for it. Other work stuff took priority, but I still want to dig in :) I was mostly posing the question anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment