Adds 'detach_from' to 'ActiveSupport::Subscriber' to detach a subscriber from a namespace. #35691

sushantmittal · 2019-03-21T15:00:25Z

Presently, If we want any logging configurable. Then, we need to do some monkey patching in log subscriber.

This pull request adds 'detach_from' to 'ActiveSupport::Subscriber'. So that, we can detach a subscriber from a namespace and then attach a custom subscriber later.

for eg: We can use custom activejob logging in this way:

ActiveJob::Logging::LogSubscriber.detach_from :active_job
CustomActiveJobLogging::LogSubscriber.attach_to :active_job

sushantmittal · 2019-03-21T15:50:13Z

I need to check why test cases of test/subscriber_test.rb failed in travis ci. They passed in my local environment.

shailesh-kalamkar · 2019-03-22T09:22:18Z

activesupport/lib/active_support/subscriber.rb

@@ -40,6 +44,22 @@ def attach_to(namespace, subscriber = new, notifier = ActiveSupport::Notificatio
        end
      end

+      # Deattach the subscriber from a namespace.
+      def deattach_from(namespace, subscriber = new, notifier = ActiveSupport::Notifications)


@sushantmittal Why is it deattach and not detach?

@shailesh-kalamkar

deattach is synonym of detach only.

detach is definitely the more common spelling of the word. It's also used in a few places in the rails codebase already, whereas deattach is not: https://github.com/rails/rails/search?q=detach&unscoped_q=detach

ok I will update it as we are using in rails already.

rafaelfranca · 2019-03-28T21:37:03Z

activesupport/lib/active_support/subscriber.rb

@@ -24,6 +24,10 @@ module ActiveSupport
  # After configured, whenever a "sql.active_record" notification is published,
  # it will properly dispatch the event (ActiveSupport::Notifications::Event) to
  # the +sql+ method.
+  #
+  # We can deattach a subscriber as well:
+


You need to add a # on this line.

ok I will update it.

rafaelfranca · 2019-03-28T21:40:36Z

activesupport/lib/active_support/subscriber.rb

+        end
+
+        def find_attached_subscriber(subscriber)
+          subscribers.find { |attached_subscriber| attached_subscriber.instance_of?(subscriber.class) }


If all we want is the class why deattach_from accept an instance? And why passing a subscriber to deattach_from if in the end we want to detach the current class?

I was thinking to implement it like attach_to. But yeah I think it makes more sense to not pass subscriber in it and use current class to detach. I will make a change. Thanks

sushantmittal · 2019-03-29T09:49:38Z

@rafaelfranca : Changes done. Please check.

activesupport/lib/active_support/subscriber.rb

…iber from a namespace.

sushantmittal · 2019-04-04T08:27:12Z

@rafaelfranca : Done suggested change in description and squashed the commits. Please check.

rails-bot bot added the activesupport label Mar 21, 2019

shailesh-kalamkar reviewed Mar 22, 2019

View reviewed changes

sushantmittal mentioned this pull request Mar 24, 2019

Added 'deattach_from' to 'ActiveSupport::Subscriber' to deattach a su… #35729

Closed

rafaelfranca reviewed Mar 28, 2019

View reviewed changes

sushantmittal force-pushed the add_deattach_from_in_active_support_subscriber branch 6 times, most recently from 4f8cb5c to fbffbce Compare March 29, 2019 09:06

sushantmittal changed the title ~~Adds 'deattach_from' to 'ActiveSupport::Subscriber' to deattach a subscriber from a namespace.~~ Adds 'detach_from' to 'ActiveSupport::Subscriber' to deattach a subscriber from a namespace. Mar 29, 2019

sushantmittal changed the title ~~Adds 'detach_from' to 'ActiveSupport::Subscriber' to deattach a subscriber from a namespace.~~ Adds 'detach_from' to 'ActiveSupport::Subscriber' to detach a subscriber from a namespace. Mar 29, 2019

rafaelfranca requested changes Apr 3, 2019

View reviewed changes

activesupport/lib/active_support/subscriber.rb Outdated Show resolved Hide resolved

Added 'detach_from' to 'ActiveSupport::Subscriber' to detach a subscr…

ca19b7f

…iber from a namespace.

sushantmittal force-pushed the add_deattach_from_in_active_support_subscriber branch from a56f7cf to ca19b7f Compare April 4, 2019 05:28

rafaelfranca merged commit dd972f9 into rails:master Apr 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds 'detach_from' to 'ActiveSupport::Subscriber' to detach a subscriber from a namespace. #35691

Adds 'detach_from' to 'ActiveSupport::Subscriber' to detach a subscriber from a namespace. #35691

sushantmittal commented Mar 21, 2019 •

edited

sushantmittal commented Mar 21, 2019

shailesh-kalamkar Mar 22, 2019

sushantmittal Mar 24, 2019

connorshea Mar 28, 2019 •

edited

sushantmittal Mar 29, 2019

rafaelfranca Mar 28, 2019

sushantmittal Mar 29, 2019

rafaelfranca Mar 28, 2019

sushantmittal Mar 29, 2019

sushantmittal commented Mar 29, 2019

sushantmittal commented Apr 4, 2019 •

edited

Adds 'detach_from' to 'ActiveSupport::Subscriber' to detach a subscriber from a namespace. #35691

Adds 'detach_from' to 'ActiveSupport::Subscriber' to detach a subscriber from a namespace. #35691

Conversation

sushantmittal commented Mar 21, 2019 • edited

sushantmittal commented Mar 21, 2019

shailesh-kalamkar Mar 22, 2019

Choose a reason for hiding this comment

sushantmittal Mar 24, 2019

Choose a reason for hiding this comment

connorshea Mar 28, 2019 • edited

Choose a reason for hiding this comment

sushantmittal Mar 29, 2019

Choose a reason for hiding this comment

rafaelfranca Mar 28, 2019

Choose a reason for hiding this comment

sushantmittal Mar 29, 2019

Choose a reason for hiding this comment

rafaelfranca Mar 28, 2019

Choose a reason for hiding this comment

sushantmittal Mar 29, 2019

Choose a reason for hiding this comment

sushantmittal commented Mar 29, 2019

sushantmittal commented Apr 4, 2019 • edited

sushantmittal commented Mar 21, 2019 •

edited

connorshea Mar 28, 2019 •

edited

sushantmittal commented Apr 4, 2019 •

edited