-
-
Notifications
You must be signed in to change notification settings - Fork 102
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
Add special case for Class.new method signature that should use initialize signature instead #419
base: main
Are you sure you want to change the base?
Conversation
…alize signature instead
def method_or_class_initialize(method) | ||
# Support only not redefined Class.new | ||
return method if !method.respond_to?(:owner) || method.owner != Class | ||
return method if !method.respond_to?(:name) || method.name != :new |
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.
What are the cases when methods don't respond to owner
or name
?
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.
At least one of the tests are passing Proc, which fails without this check.
This is the wrong place for this I feel. We handle this specifically for rspec-mocks at the time of stub creation, we do this with:
Which then calls:
Later on. |
I'd rather rspec-support did not magically return a different method handle from whats requested as thats not the purpose of it. If theres a specific missing handling of |
That was also my worry about it. It's good to know there is already a case for it in rspec-mocks, but how come it's not catching the above case? |
In case `it_behaves_like` is used a Proc is passed to the checker. If a regular method is stubbed, we pass it through. For stubbing `expect(MyClass).to receive(:new)` we actually need to stub `initialize` on the class.
Presumedly because it has never supported the above case. |
To clarify further, we added support for @pirj I'd like to have final review on this, please don't merge. |
While investigating rspec/rspec-mocks#1324 it turned out that while for most cases RSpec::Support::MethodSignature returns correct signature, for
.new
there is still some room for improvement.In Ruby native
Class::new
is a direct proxy toClass#initialize
method. As a result in cases like this signature reported by.new
is different from#initialize
, but Ruby will blame caller ofnew
, notnew
itself. You can see it using following script:With changes to Keyword arguments in 2.7 this is leading to weird situations when random libraries are blamed for disparity between
new
andinitialize
signature. This PR fixes it by reaching toinitialize
if nativeClass.new
is called - in any other case (i.e. whennew
is redefined) it will fall back to native behaviour.