Behavior change when decorating array

[cheald]

In 0.18, calling something like FooDecorator.decorate([@foo]) would return an array of decorated Foos. It now tries to decorate the array itself. Is this desired behavior?

I think this arose from aliasing decorate to new. Before, it had special behavior when passed an array.

We have collection decorator, so we could make decorate use that if passed an array, or we could add something like a Decorator::decorate_all method which expects an Enumerable.

I've worked around this in my code by using [@foo].map(&:decorate), which works fine, but the semantics are different since that doesn't ensure that I'm getting an array of FooDecorator back.

[nashby]

Yeah, we have decorate_collection but I'm not sure why decorate no longer works with collections. Any reason for this? /cc @steveklabnik

[haines]

Yep, I changed the behaviour in 7748ce5, if you use decorate_collection it will return a CollectionDecorator as you expect.

As you can see from the diff there, having decorate check if the object is a collection requires lots of nasty class-sniffing, because a simple check for respond_to? :each throws a false positive for some non-collection objects (Structs, Sequel::Models).

Apart from that implementation issue, I think it is more intention-revealing to have decorate return a singular decorator and decorate_collection return a collection decorator. In fact, the motivation for the change was that someone wanted to use a singular decorator on an Enumerable (#328), but of course decorate was decorating the items of the collection instead.

@steveklabnik This change probably needs to be documented. I can have a stab at updating the changelog and README if you want?

[cheald]

Oh, cool. I missed decorate_collection. Excellent - should be solved with the documentation update then. I completely agree that not bundling the behavior into #decorate is the less surprising route.

[steveklabnik]

Ya a changeling needs built in general. I did it sorta with my blog post,
but it's one thing to do before a real 1.0.0 release.

[nashby]

@steveklabnik well, I'm still not sure about this decision. It was pretty handy to decorate collections with the same decorate method but now I have to write this looong decorate_collection method name and worry about it's collection or not. What do you think? 😟

[steveklabnik]

Well, the real problem is that we can't satisfy both cases automatically: If you want to create a decorator that wraps a collection, or you want a collection of decorated objects.

I am not sure of the best way to resolve this. I agree that the method name is too long, but I'm not sure what to do about it.

[jasoncodes]

The magic required to detect collections is far too fallible. This alone is enough reason for me to prefer a distinct collection method.

[cheald]

I agree that we need a separate method or means of invocation.

decorate_all or decorate_many perhaps. Or we could extend Array with decorate_with, so you could [@foo].decorate_with(FooDecorator), but that feels a little upside down to me.

[steveklabnik]

decorate(thing, :as => :collection)? Dunno.

[semmons99]

decorate(thing, :as => :collection)

I like the above or decorate_each(collection)

[haines]

In defence of my bike shed :)

• decorate thing, as: :collection is more typing, not less, and goes from having two methods with distinct responsibilities to one with an if-statement.
• decorate_each sounds to me like it should return an enumerator, not an enumerable... decorate_each(things) do |decorated_thing| ...
• extending Array wouldn't work, it has to work for other types of collection too (AR lazy-loaded associations aren't arrays, for example).
• decorate_all... actually I don't mind this one! I still slightly prefer decorate_collection is because it returns a CollectionDecorator that wraps the argument, whereas decorate_all sounds to me like it maps the argument and returns an array of decorators, which is a subtle difference.

It is a relatively long method name, for sure. But it's explicit and intention-revealing and not that long.

[cheald]

There's an easy fix to make everyone happy. Just jam this in an initializer:

class << Draper::Decorator; alias dc decorate_collection; end

BAM

:D

[steveklabnik]

Ha!