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

Improve AnyInstanceMethod#hide_original_method #262

Merged
merged 2 commits into from Oct 10, 2016

Conversation

Projects
None yet
3 participants
@chrisroos
Copy link
Member

commented Sep 8, 2016

We realised that we need to make similar changes to AnyInstanceMethod#hide_original_method as were made to ClassMethod in PR #248.

This PR contains those same changes.

@chrisroos chrisroos referenced this pull request Sep 8, 2016

Closed

dry-up #248

@chrisroos chrisroos added this to the 1.2.0 milestone Sep 8, 2016

@chrisroos chrisroos force-pushed the improve-stub-any-instance-method branch 2 times, most recently from cee4a2f to 4675266 Sep 28, 2016

Simplify logic in AnyInstanceMethod#hide_original_method
This is the same change that was made to `ClassMethod` in
e87c03b.

We're now considering methods in all ancestors when determining the
visibility of the original method. This is so that we correctly set the
visibility irrespective of whether or not the method to stub is defined
on the stubbee. In particular, this means that we're now correctly
setting the visibility of stubbed methods when the original was defined
on a superclass.  This change is described by the
`assert_method_visibility` assertions in
test_should_be_able_to_stub_a_protected_superclass_method and
test_should_be_able_to_stub_a_private_superclass_method.

I've had to update four tests around ActiveRecord association behaviour
in Ruby 1.8 and 1.9. Now that we're setting visibility correctly, we
have to use `send` to call the stubbed protected and private methods.

@chrisroos chrisroos force-pushed the improve-stub-any-instance-method branch from 4675266 to 38a57c4 Sep 28, 2016

@chrisroos

This comment has been minimized.

Copy link
Member Author

commented Sep 29, 2016

@floehopper: Are you happy for this to be merged?

@chrisroos chrisroos changed the title WIP: Improve AnyInstanceMethod#hide_original_method Improve AnyInstanceMethod#hide_original_method Sep 29, 2016

@floehopper

This comment has been minimized.

Copy link
Member

commented Sep 29, 2016

Very minor: I think it would be clearer to change the 2nd commit subject line from:

"Use prepended module to stub any instance methods in Ruby 2+"

To:

"Use prepended module to stub any_instance methods in Ruby 2+

Or:

"Use prepended module to stub AnyInstance methods in Ruby 2+"

@floehopper

This comment has been minimized.

Copy link
Member

commented Sep 29, 2016

Otherwise, I'm happy for this to be merged. Good work! 👍

Use prepended module to stub AnyInstance methods in Ruby 2+
This is the same as the change made to `ClassMethod` in
43d5667.

@chrisroos chrisroos force-pushed the improve-stub-any-instance-method branch from 38a57c4 to 14b8928 Oct 3, 2016

@chrisroos

This comment has been minimized.

Copy link
Member Author

commented Oct 3, 2016

Thanks, @floehopper. I've updated the second commit message to "Use prepended module to stub AnyInstance methods in Ruby 2+". I'll merge once all the tests have passed.

@chrisroos

This comment has been minimized.

Copy link
Member Author

commented Oct 3, 2016

@grosser: Can you use the version of Mocha in this branch (improve-stub-any-instance-method) to run your test suite? We're hoping to release this version soon.

@grosser

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2016

.... oh boy ... found a 'bug' ... but honestly we use pretty funky code :)

        self.class.first_ancestor.field_list
...
NoMethodError: undefined method `field_list' for #<Mocha::ClassMethod::PrependedModule:0x0000000e813900>

... I can somehow work around that most likely ... just FYI that this changes ancestors ...

@floehopper

This comment has been minimized.

Copy link
Member

commented Oct 4, 2016

@grosser:

Thanks for running your tests. Using the prepended module approach (to intercept calls to a method) as the default approach in later versions of Ruby feels like a more elegant solution. However, I take your point that it does mean the temporary addition of a module to the ancestor chain.

I'm happy that the change to the ancestor chain ought not cause problems for idiomatic/duck-typing code. Are you happy with this justification? We'd be happy to try to help fix your code to avoid the problem.

Did you see any other test failures? Are you otherwise happy for the latest code in master to be released?

@grosser

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2016

yes, sounds like a good solution ... did not see any other issues!

@chrisroos chrisroos merged commit ae3d6d8 into master Oct 10, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@chrisroos chrisroos deleted the improve-stub-any-instance-method branch Oct 10, 2016

@floehopper

This comment has been minimized.

Copy link
Member

commented Oct 10, 2016

@grosser: Mocha v1.2.0 has just been released incorporating these changes. Thanks for your help & patience!

@grosser

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2016

thx for fixing, already bumped :D

On Mon, Oct 10, 2016 at 8:18 AM, James Mead notifications@github.com
wrote:

@grosser https://github.com/grosser: Mocha v1.2.0
https://rubygems.org/gems/mocha/versions/1.2.0 has just been released
incorporating these changes. Thanks for your help & patience!


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#262 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAAsZ_wG29WnnuI1Urj4LpAF_4mtbvsRks5qylczgaJpZM4J31j1
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.