Sketch of fix for delegate private method stubbing #63
Conversation
|
Thanks for taking a stab at this. I wonder if it's sufficient to silence the warning itself? As far as I can tell, the behaviors in your specs all pass except for the "should not warn" w/o your fix in lib. That might be a much simpler solution. Thoughts? |
|
|
||
| it "does not issue a warning when stubbing the private kernel method" do | ||
| hash = subject.new({}) | ||
| expect(hash).not_to receive(:warn) |
There was a problem hiding this comment.
I think I'd rather see this use the new output matcher rather than mocking:
expect {
allow(hash).to receive(:rand).and_call_original
}.not_to output.to_stderrThe mock expectation is more brittle as it assumes a warning implementation that calls warn on the object itself when in fact warn can be called on any object (or even directly on Kernel) in order to produce a warning.
There was a problem hiding this comment.
I was wondering about that, I knew the matcher existed but couldn't remember what it was called and asked a few other maintainers. That's definitely what I had intended.
We considered that. This solution seems like it's a bit overwrought. Are there any dangers from just silencing the warning? |
|
@myronmarston I'm not sure which is more brittle: silencing the warming (possibly hiding another if something else is funky) or this crazy metaprogramming. Also: do you have an opinion on which implementation of |
| @@ -42,7 +42,42 @@ def self.define_optimized_require_for_rspec(lib, &require_relative) | |||
| # - BasicObject subclasses that mixin a Kernel dup (e.g. SimpleDelegator) | |||
| if RUBY_VERSION.to_i >= 2 && RUBY_ENGINE != 'rbx' | |||
| def self.method_handle_for(object, method_name) | |||
| KERNEL_METHOD_METHOD.bind(object).call(method_name) | |||
| if object_is_a_delegator?(object) && kernel_has_private_method?(method_name) && !object_has_own_method?(object, method_name) | |||
There was a problem hiding this comment.
Long line is long and this makes me sad :(
I can't think of any dangers, although we may want to silence the warning in a way that it only silences this specific warning and not other warnings (such as temporarily changing I'm also thinking that if we're going to go with the warning-silencing route (which I would advocate for at this point), it might be best to leave the code here as-is (w/o the changes in this PR) and then wrap the call from rspec-mocks to the method here in the warning-silencing method. In fact, if we do that, I'm not sure that we'll need the fancy |
|
Should I close this in favour of a PR that simply silences the warning? |
that sounds good to me. |
|
Was this issue ever fixed? Since I installed Ruby 2.3, I'm having a lot of If I define the method in the subclass of the warning disappears. |
A sketch of a fix for rspec/rspec#42
I paired on this with @bestie. It seems to solve the problem, but I'm really not sure how solid this is a solution. The basic problem here is that when asking a delegator if it has a particular method, it raises a warning if that method is a private method that is defined on
Kernel. The solution is to check if the delegator object itself implements that method, and if it does, take that implementation. Otherwise we take the implementation off kernel itself. I'm not sure how I feel about this at all, but the fix does seem to deal with the issue.