refactor: move module query-methods into collection-decorator #874
Conversation
|
|
||
| # Configures the strategy used to proxy the query methods, which defaults to `:active_record`. | ||
| def strategy | ||
| @strategy ||= LoadStrategy.new(Draper.default_query_methods_strategy) |
There was a problem hiding this comment.
Does the name LoadStrategy still make sense in this new context? Should it be something like QueryMethodStrategy or something more specific to query methods? I don't have a big opinion here, but it does seem odd.
|
@brunohkbx can you please rebase your branch on master ? Thank you ! |
Alexander-Senko
left a comment
Alexander-Senko
left a comment
There was a problem hiding this comment.
Decorated collection may be not backed by a DB. These strategies make no sense in that case (see #920).
|
|
||
| # Proxies missing query methods to the source class if the strategy allows. | ||
| def method_missing(method, *args, &block) | ||
| return super unless strategy.allowed? method |
There was a problem hiding this comment.
I'm new here, so please forgive my stupid question 🙏
Why do we need any strategy in the first place? Isn't object.respond_to? enough?
There was a problem hiding this comment.
I see: it's for methods returning decoratable collections. It's not obvious for everyone, though (see #867).
|
|
||
| # Proxies missing query methods to the source class if the strategy allows. | ||
| def method_missing(method, *args, &block) | ||
| return super unless strategy.allowed? method |
There was a problem hiding this comment.
| return super unless strategy.allowed? method | |
| return super unless object.respond_to? method |
| def method_missing(method, *args, &block) | ||
| return super unless strategy.allowed? method | ||
|
|
||
| object.send(method, *args, &block).decorate(with: decorator_class, context: context) |
There was a problem hiding this comment.
| object.send(method, *args, &block).decorate(with: decorator_class, context: context) | |
| object.send(method, *args, &block).try(:decorate, with: decorator_class, context: context) |
4 participants
Description
As we discussed on #868, it makes sense to move this module into CollectionDecorator since it depends upon
decorator_classandcontext.References