Add wrap_with to Relation; simplify decorator / presenter / etc. code

[al2o3cr]

The attached code adds a wrap_with method to Relation that wraps objects retrieved from the relation in the specified class. A trivial example:

class UserWrapper < DelegateClass(User)
  def magic_thing
    puts 'wat'
  end
end

User.where(:name => 'Bob').wrap_with(UserWrapper).first.magic_thing # => prints 'wat'

A short test is included - suggestions for additional testing (or more meaningful testing) are welcome.

There are also a number of additional features that could be included here, if they make sense:

• wrapping with additional classes may be useful; currently, chaining another wrap_with call overwrites the value set in the first one. This would be a simple change, given the existing plumbing for dealing with multi-valued data on Relation.
• passing additional arguments to the method could be useful; for instance, some_relation.wrap_with(SomeClass, :foo, :bar, :baz) would pass the additional arguments on to the constructor, so each record in some_relation would be wrapped via SomeClass.new(record, :foo, :bar, :baz). Again, a fairly trivial modification.
• for extreme cases, passing a block may make more sense. For instance:

some_relation.wrap_with(SomeClass) { |some_class| some_class.do_something }
# which wraps as:
SomeClass.new(record).tap { |some_class| some_class.do_something }

• of course, there's also an argument that a zero-arg block version might make sense:

some_relation.wrap_with { |record| SomeClass.new(record) }

Here, wrap_with is just calling the block with the record directly.

• the current code doesn't do anything when building / creating new records; it was extracted from a live app that used wrap_with solely for read-only views. I'm not certain what the desirable behavior is here.

[rafaelfranca]

cc @jonleighton

[rafaelfranca]

I don't think this should be handled by Active Record. I'm 👎 for this feature.

[carlosantoniodasilva]

Hey @al2o3cr, I particularly don't think it's AR's responsibility (or Relation) to handle wrapping to another objects. When one instantiate an Active Record object, ie with first, he'd expect to get back an AR instance, not a wrapped object.

So, my 2 cents are that there's no real gain baking this into Rails, when we can be explicit about the wrapping whenever necessary, with your example:

User.where(:name => 'Bob').wrap_with(UserWrapper).first.magic_thing
#vs
UserWrapper.new(User.where(:name => 'Bob').first).magic_thing

User.where(:name => 'Bob').wrap_with(UserWrapper)
# vs
User.where(:name => 'Bob').map { |u| UserWrapper.new(u) }

Thanks!

[al2o3cr]

@carlosantoniodasilva It's only practical to perform the transformation you've described when the code using objects from the relation and the code setting up the relation are together - otherwise, you wind up with the wrapping code sprinkled everywhere.

In my app, the code that built the relation needed to conditionally wrap the returned objects for callers that might either paginate the relation or call find(id) against it; this method allowed that decision to be made in one place rather than in each action that used the relation.

A more concrete example would be functionality like Draper's decorates_association which would be greatly simplified with wrap_with. For instance, the top commit on that repo is a revert of an attempt to deal with the need to wrap relations.

/cc @steveklabnik - would love to hear your thoughts, as (presumably) a strong proponent of decorators.

[steveklabnik]

Well, yes, I am, but I don't see what this feature gives us over .collect{|x| Decorator.new(x)}, really. I am also unsure that it's AR's job to worry about being decorate-able.

That said, AR stuff to make my life maintaining Draper is nice, as you've noted, wrapping can be hard at times.

[carlosantoniodasilva]

I understand it may make some use cases easier to deal with, but I still don't think, as @steveklabnik commented, that's AR's job to handle wrapping itself. I'd rather stick with map/collect because I think it's more intention revealing and easier for someone new to the code, since it's a very simple and well known structure.

Anyway, lets wait for some more people to give feedback. Thanks @al2o3cr!

[evanphx]

I agree with @steveklabnik that this is unnecessary when it is just a .map call. If this is a common need for you, I'd reverse it and put the code on your UserWrapper so you do UserWrapper.from User.where(:name => "Bob") then you've got control over how it works and it's easier to understand because the primary noun (UserWrapper) is on the left.

[srbaker]

This is just another example of Rails re-implementing Ruby with weird syntax. It’s depressing.

Specifically, two problems:

1. It duplicates existing functionality, and not only does it not provide anything extra, it's considerably less flexible than simply instantiating an object and passing in dependencies (as shown in others' examples).
2. It means that new users have to learn "the Rails way" and "the Ruby way", learn the differences, and learn which situations are appropriate for each.

[srbaker]

I have a question, and I'm not sure this is the appropriate place to ask, but I'd really like to know the answer (or at least have a better understanding).

Why do we wind up with things like this being suggested (and often added) to Rails? Do people not understand Ruby, OO, good design? What is the benefit of divorcing Rails from Ruby? Do people find Ruby a difficult programming language? Is the goal here to get more non-programmers writing Rails?

(I'm genuinely curious, because I feel that I have to understand the cause problem before I can help stop it.)

[steveklabnik]

@srbaker that kind of question would be an appropriate post to the rails-core mailing list.

[srbaker]

It probably is. I'm having second thoughts about whether I care to hear the "answers" or not.

[pedrosnk]

Personally I don't think AR should be responsable to handle it.

Specially when all you need to do is use some map/collect to handle it.

[barelyknown]

Agree with @carlosantoniodasilva that ActiveRecord should return ActiveRecord instances and with @steveklabnik et al that collect() works fine as is for iterating and wrapping.

[jredville]

I agree with the rest of the comments here that this is outside the responsibility of AR, and probably outside the responsibility of Rails in general.

As a pattern, I feel that @evanphx's concept of having the decorator be in charge (instead of the finder) is cleaner and more appropriate. The fact that Evan's would work across ORM's is probably a signal of this.

[xentek]

👎 this would make a cool gem, but I'd hate to see it in core for all the reasons above and more

[steveklabnik]

Seems like almost everyone agrees that this isn't a good idea. If someone on core feels differently, please re-open and merge.