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

Delegate to private methods #531

Closed
wants to merge 2 commits into from

Conversation

mrageh
Copy link

@mrageh mrageh commented May 26, 2014

Issue #526
Previously if a class delegated to a private method the matches? method would
return false. This was becauserespond_to? always returns false for private methods, this commit fixes this by making respond_to? evaluate private methods.

post_office = PostOffice.new
message = 'Expected PostOffice#deliver_mail not to delegate to PostOffice#mailman, but it did'

expect {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid using {...} for multi-line blocks.

@mcmire
Copy link
Collaborator

mcmire commented May 26, 2014

Sweet, thank you. I'll take a closer look at this when I get a chance.

@mrageh
Copy link
Author

mrageh commented May 26, 2014

It was fun @mcmire, this is one of my favourite (favorite in the States) gems. I want to help with as many bug fixes as possible.

@mcmire
Copy link
Collaborator

mcmire commented May 26, 2014

@Adam89 Glad to hear! There are definitely plenty of bugs to fix and areas of the code to work on for sure.

@mrageh
Copy link
Author

mrageh commented Jun 3, 2014

@mcmire I just thought of a edge case and am not sure this implementation would work for private methods that take arguments. The private method would be found but I think the args won't get passed through to the object.

@mcmire
Copy link
Collaborator

mcmire commented Jun 3, 2014

It should still work. The target method actually gets replaced with a method double, and the arguments given to that method get captured. (This is how we determine that the method got delegated.)

@mrageh
Copy link
Author

mrageh commented Jun 3, 2014

@mcmire could you check if this is ready to be merged so I can start to focus on other bugs?

expect(post_office).to delegate_method(:deliver_mail).to(:mailman)
end

it 'produces the correct failure message if the assertion was negated' do
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't actually know if we need this test here, I'd be fine with leaving this out since we already test this in the case that the method is public.

@mcmire
Copy link
Collaborator

mcmire commented Jun 3, 2014

Sorry @Adam89, thanks for nudging me. Just had a couple of things and then it should be good to merge.

@mcmire mcmire added this to the 2.7.0 milestone Jun 4, 2014
@mrageh
Copy link
Author

mrageh commented Jun 4, 2014

@mcmire does It look ready to be merged now?

@mrageh
Copy link
Author

mrageh commented Jun 4, 2014

Just seen the label I'll squash the commits!!

@mcmire
Copy link
Collaborator

mcmire commented Jun 4, 2014

Cool. Thanks. When I get time, I'll merge this -- there are some things for 2.6.2 that need to go out first.

Adam89 added 2 commits June 14, 2014 13:23
Issue #526
Previously if a class delegated to a private method the `matches?` method would
return `false`. This was because`respond_to?` always returns `false` for private methods, this commit fixes this by making `respond_to?` evaluate private methods.
@mcmire
Copy link
Collaborator

mcmire commented Jul 26, 2014

Thanks for this @Adam89! Merged in b639f83.

@mcmire mcmire closed this Jul 26, 2014
@aripollak
Copy link

Any chance of a new gem release for this fix? 😁

@mcmire
Copy link
Collaborator

mcmire commented Sep 3, 2014

Yes :) 2.7.0 is now released!

@freemanoid
Copy link

👍

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

Successfully merging this pull request may close these issues.

None yet

5 participants