Updating matcher to support testing class callbacks #3

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
2 participants
@pvertenten
Contributor

pvertenten commented Feb 13, 2014

Added support for object based callbacks in addition to the already supported :symbol callbacks.

So now both of the following will work.

after_create :track_activity
it { should callback(:track_activity).after(:create) }

and

after_create ActivityTracker.new
it { should callback(ActivityTracker).after(:create) }

I am possibly using some undocumented apis, but used respond_to? to guard against api changes.

Anyway, I am sending you this pull request to get the conversation started about incorporating this if that is what you end up wanting.

Still would need to add tests and update the documentation, and it could be made to further test items on the callback class as well.

@beatrichartz

This comment has been minimized.

Show comment
Hide comment
@beatrichartz

beatrichartz Feb 14, 2014

Collaborator

Thanks for the pr, I'll have a look at this in the next days!

Collaborator

beatrichartz commented Feb 14, 2014

Thanks for the pr, I'll have a look at this in the next days!

@beatrichartz

This comment has been minimized.

Show comment
Hide comment
@beatrichartz

beatrichartz Feb 15, 2014

Collaborator

Ok, the suggested API is looking good to go.

As you said, there need to be tests and documentation for this example, you can also update the readme if you like. Concerning the implementation, I think the best way to handle the added matcher complexity is to extract it into private methods to improve readability.

Looking forward to merging this!

Collaborator

beatrichartz commented Feb 15, 2014

Ok, the suggested API is looking good to go.

As you said, there need to be tests and documentation for this example, you can also update the readme if you like. Concerning the implementation, I think the best way to handle the added matcher complexity is to extract it into private methods to improve readability.

Looking forward to merging this!

@pvertenten

This comment has been minimized.

Show comment
Hide comment
@pvertenten

pvertenten Mar 14, 2014

Contributor

Closing. I sent another pull request with all the updates

Contributor

pvertenten commented Mar 14, 2014

Closing. I sent another pull request with all the updates

@pvertenten pvertenten closed this Mar 14, 2014

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