Allow extra arguments for Observers #6063

Merged
merged 1 commit into from Apr 30, 2012

Projects

None yet

6 participants

@marcandre

I wanted to use observers to send custom events and noticed that #notify_observers only accepts one argument.

Unless there's a good reason not too, it woud be nice to accept extra arguments and pass them on.

Thanks

@ahawkins

I welcome this change. I've ran into this issue before. This is definitely handy when making your own custom events.

@jeremyf

All of the tests pass against master. I also like the idea of extra params.

@rafaelfranca Its clear to merge.

@rafaelfranca rafaelfranca and 1 other commented on an outdated diff Apr 30, 2012
activemodel/lib/active_model/observing.rb
@@ -229,10 +229,10 @@ def observed_classes #:nodoc:
# Send observed_method(object) if the method exists and
# the observer is enabled for the given object's class.
- def update(observed_method, object, &block) #:nodoc:
+ def update(observed_method, object, *extra_args, &block) #:nodoc:
@rafaelfranca
rafaelfranca Apr 30, 2012

extra whitespace before the &block

@marcandre
marcandre Apr 30, 2012

Good eye. Fixed.

@rafaelfranca
Ruby on Rails member
@jeremy
Ruby on Rails member

👍

That matches Ruby's Observable also.

Worth updating the docs to make it clear how the observer method is called.

@marcandre

I've tried supplementing the doc.

Also, notify_observers is made public; this conforms to the stdlib and there's no reason I could see that it was private in the first place.

@ahawkins

@marcandre other objects should not be able to notify another object's observers. #notify_observers should be for internal use only.

@marcandre

Removed the public part, so this can be merged asap. Will open a new pull request for "public"

@rafaelfranca
Ruby on Rails member

@marcandre could you squash these commits in one? Also don't change the indentation of the private methods. The convention is indent after private/protected. See the contributing guides

@jeremy jeremy was assigned Apr 30, 2012
@tenderlove tenderlove merged commit 206b43a into rails:master Apr 30, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment