notify_observers should be public #6093

Merged
merged 1 commit into from May 1, 2012

Conversation

Projects
None yet
5 participants
Contributor

marcandre commented Apr 30, 2012

As @twinturbo objected in pull request #6063, here's a separate request for this change and expanded reasoning.

Reasons to make notify_observers public:

  • consistent with the fact that notify_observers in the stdlib's Observable is public.

  • consistent with the fact that the singleton method notify_observers is public:

    Foo.notify_observers(:my_event, the_foo) # => currently ok
    the_foo.notify_observers(:my_event) # => currently not ok

  • notify_observers is not the right level to restrict access. Some actions would be best kept private (say :after_save) but custom actions could very well be triggered by external clients. The class and its instances simply act as a rendez-vous point where complete strangers to the class can communicate.

Example:

An observer implements :after_save to detect some state change. If detected, it wants to send a custom notification :some_state_changed, so other observers can be notified and don't have to reimplement the state change logic. It shouldn't have to use send.

Thanks

activemodel/lib/active_model/observing.rb
@@ -109,7 +109,6 @@ def inherited(subclass)
end
end
- private
# Fires notifications to model's observers
#
# def save
@carlosantoniodasilva

carlosantoniodasilva Apr 30, 2012

Owner

I think you could review the indentation here, and also check if there are tests where you could remove send calls, and if there aren't, write at least one test case, otherwise this could be make private again by mistake in the future. Thanks!

@marcandre

marcandre Apr 30, 2012

Contributor

done

An observer implements :after_save to detect some state change. If detected, it wants to send a custom notification :some_state_changed, so other observers can be notified and don't have to reimplement the state change logic. It shouldn't have to use send.

On observer should only observe. It should not notify other observers. Observers are own way event consumption.

@ghost ghost assigned jeremy Apr 30, 2012

Owner

rafaelfranca commented Apr 30, 2012

cc/ @jeremy

Thanks, but the pull request does not apply cleanly anymore, could you please rebase from master?

Contributor

marcandre commented May 1, 2012

Sure, done. Strange we don't get notification when a pull request is closed.
Thanks

jeremy added a commit that referenced this pull request May 1, 2012

Merge pull request #6093 from marcandre/observer_public
notify_observers should be public

@jeremy jeremy merged commit cb5b0cf into rails:master May 1, 2012

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