-
Notifications
You must be signed in to change notification settings - Fork 21.6k
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
Document Module#delegate_missing_to in the guides #29268
Document Module#delegate_missing_to in the guides #29268
Conversation
r? @matthewd (@rails-bot has picked a reviewer for you, use r? to override) |
variables, methods, constants, etc. | ||
|
||
WARNING: The delegated method calls must be public on the target, otherwise | ||
they will raise `NoMethodError`. |
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.
This warning demonstrates that the behaviour doesn't match your manual implementation. 😉
Really though, I think the full method_missing
/respond_to_missing?
illustration is too much unnecessary detail. It works for delegate
, but isn't an efficient use of space in general. Let's ditch it, and just focus on usage.
Maybe also consider reusing the User/Profile example... "or you could just forward everything" seems a logical leap from "here's how to delegate 4 methods in one line"
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.
Sure, I will try that! I :)
@matthewd I have condensed the example to the one above. Are you okay with it? |
fb6d450
to
7ee6728
Compare
#### `delegate_missing_to` | ||
|
||
Imagine you would like to delegate all of the `Profile` methods to the `User` | ||
object. Instead of listing every `Profile` method in a `delegate` call, you can |
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.
delegate all of the
Profile
methods to theUser
object
That sounds around the wrong way -- though the exact opposite isn't right either.
'to' -> 'from' might be sufficient?
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.
Updated it to:
Imagine you would like to delegate all methods from the
Profile
to theUser
object...
Does it feel better now?
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.
No, because that still doesn't match what the code says: User.delegate_missing_to :profile
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.
So another take, this time trying to go with what the code says. 😅
Imagine you would like to delegate everything missing from the
User
object,
to theProfile
one. Thedelegate_missing_to
macro let's you implement this
in a breeze:
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.
☝🏻 looks good
("lets" not "let's")
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.
✅
fa785fa
to
dced883
Compare
The target can be anything callable within the object, e.g. instance variables, | ||
methods, constants, etc. | ||
|
||
NOTE: Defined in `active_support/core_ext/module/delegation.rb` |
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.
lgtm. We should mention only public methods are delegated.
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.
Just dropped it in a sentence. If you think a warning, like the one in #29268 (comment) works better, ping me and I'll change it.
Added a small section for it in the `Active Support Core Extensions` guide. [ci skip]
dced883
to
43512df
Compare
Added a small section for it in the
Active Support Core Extensions
guide.