-
Notifications
You must be signed in to change notification settings - Fork 605
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
Implement {Method,UnboundMethod}#super_method #3472
Conversation
I can't imagine we don't have anything in place that would make supporting this easier without having to manually track and flush ancestors in Method/UnboundMethod objects. @brixen does anything come to mind? |
@@ -158,6 +159,23 @@ def unbind | |||
UnboundMethod.new(@defined_in, @executable, @receiver.class, @name) | |||
end | |||
|
|||
def super_method | |||
@ancestors.each_with_index do |a, i| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an aside, please don't use one letter variables. I know the codebase is littered with them but it's a huge pain to constantly try and remember what a
or x
or another-one-letter-variable stands for.
Maybe it helps to start with the meaning of |
@robin850 - I agree with @yorickpeterse that it would be best not to use the direct_superclass = @defined_in.direct_superclass
if direct_superclass
mod, entry = direct_superclass.lookup_method(method_name)
# Use mod and entry to construct the object to return
end
There is a spec that this PR is fulfilling (see the removed fail tags). With that said, the spec looks a bit sparse, so @robin850, if you know more about the behavior of |
The implementation relies on Module#direct_superclass to run through the whole ancestry chain as #superclass would not include the modules. The method is about 2 times slower than the MRI implementation though. Benchmark: http://git.io/vmhpf.
Hey, thank you very much for your quick comments guys! This looks way better with your proposal @jemc, the
At least there were no specs for the case when it returns This would be cool to test the implementation against a project but It looks like this method is not yet widely used doing a quick search on GitHub ; at least this test passes in the Active Support test suite. |
Thanks, @robin850 - looks much better to me. I'll leave it to @yorickpeterse to merge when he also approves. |
method.super_method.should == nil | ||
end | ||
|
||
it "doesn't look for removed methods" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd change this to it 'returns nil when the method is removed'
or something along those lines. "doesn't look for..." doesn't really indicate that super_method
returns in this case.
Other than my little spec description remark this looks fine to me, great work! |
Fair point! 👍 The patch has been updated, thanks for your quick review guys! |
Based on commit ruby/ruby@b4981594.
Sweet, thanks! |
Hello,
This is a tiny pull request that adds the
#super_method
method to theMethod
andUnboundMethod
objects.With this feature added, the method objects need to keep track of the ancestors chain. This is not a problem when the chain only contain parents of the class. This is, however, when a module is included inside it since we loose the previous ancestry chain.
The method is about 5 times slower than the MRI implementation though. I don't know if it's acceptable.
Cross-refs #3264.
Have a nice day.