Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

@sushantmittal
Copy link
Contributor

@sushantmittal sushantmittal commented Mar 21, 2019

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
Copy link
Contributor Author

@sushantmittal sushantmittal commented Mar 21, 2019

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

@@ -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)
Copy link
Contributor

@shailesh-kalamkar shailesh-kalamkar Mar 22, 2019

@sushantmittal Why is it deattach and not detach?

Copy link
Contributor Author

@sushantmittal sushantmittal Mar 24, 2019

@shailesh-kalamkar

deattach is synonym of detach only.

Copy link
Contributor

@connorshea connorshea Mar 28, 2019

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

Copy link
Contributor Author

@sushantmittal sushantmittal Mar 29, 2019

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

@@ -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:

Copy link
Member

@rafaelfranca rafaelfranca Mar 28, 2019

You need to add a # on this line.

Copy link
Contributor Author

@sushantmittal sushantmittal Mar 29, 2019

ok I will update it.

end

def find_attached_subscriber(subscriber)
subscribers.find { |attached_subscriber| attached_subscriber.instance_of?(subscriber.class) }
Copy link
Member

@rafaelfranca rafaelfranca Mar 28, 2019

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?

Copy link
Contributor Author

@sushantmittal sushantmittal Mar 29, 2019

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 sushantmittal force-pushed the add_deattach_from_in_active_support_subscriber branch 6 times, most recently from 4f8cb5c to fbffbce Mar 29, 2019
@sushantmittal
Copy link
Contributor Author

@sushantmittal sushantmittal commented Mar 29, 2019

@rafaelfranca : Changes done. Please check.

@sushantmittal 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 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
@sushantmittal sushantmittal force-pushed the add_deattach_from_in_active_support_subscriber branch from a56f7cf to ca19b7f Apr 4, 2019
@sushantmittal
Copy link
Contributor Author

@sushantmittal sushantmittal commented Apr 4, 2019

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

@rafaelfranca rafaelfranca merged commit dd972f9 into rails:master Apr 4, 2019
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants