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.
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.
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_methodmethod to theMethodandUnboundMethodobjects.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.