Remove .decorates method and all class method proxying

[haines]

I thought I'd throw this out there and see what people thought.

@steveklabnik has mentioned a few times about removing .decorates, and letting one decorator decorate multiple models. I like the idea of decoupling the decorator from the model class as much as possible.

An important consequence of this is that it would no longer be possible to proxy class methods, in particular ProductDecorator.all, ProductDecorator.find and friends.

Personally I think this is a good thing, I've never been very comfortable with that syntax and have preferred ProductDecorator.decorate(Product.all) because, conceptually, the idea of "finding a ProductDecorator" doesn't sit well with me compared to "finding a Product and decorating it".

Thoughts?

[steveklabnik]

Well.....

So there's a big schism here. One is 'pretend to be like AR as much as possible.' The other is 'follow good OO design.' ;)

What I'd like for 2.0 (yes, I know we're not at 1.0 yet ;) ) is to build a core of 'good oo' stuff and then add a layer of 'ar compatible stuff' on top. So that people can do both.

[haines]

Hmmm yes fair enough. How about something like

class ProductDecorator < Draper::Decorator
  # inferred model
  add_finders

  # explicit model
  add_finders for: :widget
end

Then the AR stuff could be moved into a module that was extended on demand? Then it would be possible to write some decorators that could deal with multiple classes, and some that were tightly bound to one.

Edit In fact, going back to composition over inheritance, I could imagine add_finder creating a Finder object that the decorator could delegate to :)

[steveklabnik]

Hmmmmm. I kinda like that, actually. Sounds pretty reasonable.

[haines]

Awesome! I'll have a crack at this tomorrow (I'm on UK time).

[haines]

See what you think, I ended up not creating a new object on the basis that the methods we are dynamically adding are already simple delegates to another object.

[steveklabnik]

This looks great. I only want to have one more bikeshed: I'm not sure that add_finders is a great name. Rails' class macros are all declarative based, and even though it's literally adding the finders, it feels odd to me since it's not a declarative style name.

I'm not sure what it should be, though.

[stevenharman]

Just spitballing here...

class ProductDecorator < Draper::Decorator
  can_find :product

  finds :product

  finds_model :product

  findable :product
end

Not sure I love any of them.

[collin]

Just gonna toss something at the wall cause I walked in here from twitter and 🍷

class MyFancyUserDecorator < UserDecorator
  scope User.fancy
end

[steveklabnik]

Hmmmmmmmmmm

[lest]

👍 to removing .decorates, 👎 to coupling finders into draper

[haines]

@steveklabnik No worries, I didn't like it much either but couldn't come up with anything better. And we're going to have to live with our bikesheds once this goes 1.0 - let's paint them properly :)

@stevenharman My favourite of those is finds :product, except I don't think that simply writing finds looks right when you want the model to be inferred. We could just make the model name required for the sake of nicer syntax, since decorates doesn't infer models either. finds_model is my pick of the rest, and looks ok without an explicit model.

Let me put another one up for consideration too: has_finders / has_finders for: :product

@lest At the moment they are right there in the Decorator, like 'em or not. This decouples them and results in a nice compromise - users can delete decorates and forget the finders ever existed, or replace decorates with [whatever we decide to call it] and keep on like nothing changed.

[steveklabnik]

@lest yeah, @haines is correct, this code is already included everywhere by default, there's no extra coupling going on here.

[haines]

Hey @steveklabnik, I went with has_finders, how does that look?

When I went to rebase, the specs were a bit of a nightmare, as there were a lot of methods on CollectionDecorator in the specs for Decorator. I've gone through and had a bit of a clean up. I moved some stuff round and reconciled some of the syntax - different authors have been using context and describe in different ways, eq vs ==, and so on.

[steveklabnik]

Looks good, thanks. I'd rather have the feature than continue the bikeshed. :) And that name is 'good enough'!