-
Notifications
You must be signed in to change notification settings - Fork 154
Feature: Handle symbol-to-proc wrappers (&:) that refer to a method created using delegation or method missing
#406
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -44,7 +44,7 @@ Implement your feature or bug fix. | |||||
|
|
||||||
| Ruby style is enforced with [Rubocop](https://github.com/bbatsov/rubocop), run `bundle exec rubocop` and fix any style issues highlighted. | ||||||
|
|
||||||
| Make sure that `bundle exec rake` completes without errors. | ||||||
| Make sure that `bundle exec rake` and `rubocop` completes without errors. | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Subject-verb agreement: two subjects -> complete. Also the line above already instructs users to run bundle exec rubocop, so this becomes a near-duplicate 🤷 |
||||||
|
|
||||||
| #### Write Documentation | ||||||
|
|
||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -548,7 +548,8 @@ def ensure_block_arity!(block) | |
| end | ||
|
|
||
| arity = object.method(origin_method_name).arity | ||
| return if arity.zero? | ||
| # functions defined using `delegate` or `method_missing` have an arity of -1 | ||
| return if arity <= 0 | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is too permissive and reintroduces the bug class #389 was meant to prevent. Ruby's negative arities encode Worth noting: Suggested fix is match the actual intent ("no required positional args"): arity = object.method(origin_method_name).arity
required = arity >= 0 ? arity : -arity - 1
return if required.zero? |
||
|
|
||
| raise ArgumentError, <<~MSG | ||
| Cannot use `&:#{origin_method_name}` because that method expects #{arity} argument#{'s' if arity != 1}. | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -400,6 +400,14 @@ class BogusEntity < Grape::Entity | |||
|
|
||||
| describe 'blocks' do | ||||
| class SomeObject | ||||
| class SomeObjectDelegate | ||||
| def method_using_delegation | ||||
| 'delegated-result' | ||||
| end | ||||
| end | ||||
|
|
||||
| delegate :method_using_delegation, to: :delegate_object | ||||
|
|
||||
| def method_without_args | ||||
| 'result' | ||||
| end | ||||
|
|
@@ -415,6 +423,23 @@ def method_with_multiple_args(_object, _options) | |||
| def raises_argument_error | ||||
| raise ArgumentError, 'something different' | ||||
| end | ||||
|
|
||||
| def method_missing(method, ...) | ||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rather than adding Something like: SomeObjectWithMissing = Class.new(SomeObject) do
def method_missing(method, ...)
return 'missing-result' if method == :method_using_missing
super
end
def respond_to_missing?(method, include_private = false)
method == :method_using_missing || super
end
end
SomeObjectWithDelegation = Class.new(SomeObject) do
InnerDelegate = Class.new { def delegated_method = 'delegated-result' }
delegate :delegated_method, to: :inner
def inner = @inner ||= InnerDelegate.new
endand specs kind of: context 'with &: referencing a method_missing-backed method' do
specify do
subject.expose :using_missing, &:method_using_missing
object = SomeObjectWithMissing.new
expect(subject.represent(object).value_for(:using_missing)).to eq('missing-result')
end
end
context 'with &: referencing a delegated method' do
specify do
subject.expose :using_delegation, &:delegated_method
object = SomeObjectWithDelegation.new
expect(subject.represent(object).value_for(:using_delegation)).to eq('delegated-result')
end
endwith the regression case that pins the boundary: context 'with &: referencing a method with required args and a splat (arity -2)' do
specify do
subject.expose :x, &:method_with_required_and_splat
object = SomeObjectWithMissing.new # or any object with such a method
expect do
subject.represent(object).value_for(:x)
end.to raise_error(ArgumentError, match(/method expects/))
end
end |
||||
| return 'missing-result' if method.to_sym == :method_using_missing | ||||
|
|
||||
| super | ||||
| end | ||||
|
|
||||
| def delegate_object | ||||
| @delegate_object ||= SomeObjectDelegate.new | ||||
| end | ||||
|
|
||||
| private | ||||
|
|
||||
| def respond_to_missing?(method, include_private = false) # rubocop:disable Style/OptionalBooleanParameter | ||||
| method.to_sym == :method_using_missing || | ||||
| super | ||||
| end | ||||
| end | ||||
|
|
||||
| describe 'with block passed in' do | ||||
|
|
@@ -459,6 +484,28 @@ def raises_argument_error | |||
| end | ||||
| end | ||||
|
|
||||
| context 'with block passed in via & that uses `missing_method`' do | ||||
| specify do | ||||
| subject.expose :using_missing, &:method_using_missing | ||||
|
|
||||
| object = SomeObject.new | ||||
| expect(object.method(:method_using_missing).arity).to eq(-1) | ||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The
Suggested change
|
||||
| value = subject.represent(object).value_for(:using_missing) | ||||
| expect(value).to eq('missing-result') | ||||
| end | ||||
| end | ||||
|
|
||||
| context 'with block passed in via & that uses `delegate`' do | ||||
| specify do | ||||
| subject.expose :using_delegation, &:method_using_delegation | ||||
|
|
||||
| object = SomeObject.new | ||||
| expect(object.method(:method_using_delegation).arity).to eq(-1) | ||||
| value = subject.represent(object).value_for(:using_delegation) | ||||
| expect(value).to eq('delegated-result') | ||||
| end | ||||
| end | ||||
|
|
||||
| context 'with block passed in via &' do | ||||
| specify do | ||||
| subject.expose :that_method_with_one_arg, &:method_with_one_arg | ||||
|
|
||||
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.
The entry is placed above the
#### Features/#### Fixesheaders rather than under one of them. Please move the line under#### Fixes.