.decorate is the Devil

[jcasimir]

This is the first line of code in the README:

Article.find(params[:id]).decorate

And I hate it. Scroll through the closed issues and you'll find many of them are centered around making this feature work. It has to guesstimate (infer) what the matching decorator class is. It has to hook into the underlying ORM. It has to take different actions when there are chained methods or associations involved. It's trying to do too much.

I've been writing and teaching Ruby and Rails for ten years now. One lesson I've learned over and over and over is "explicit over implicit". And this is a perfect example. The library implementation would be much easier and less buggy if the above were replaced with this:

ArticleDecorator.new( Article.find(params[:id])

Is it cute? No. Is the intention and execution clear? Absolutely.

Is there a compromise? I still like this syntax:

ArticleDecorator.find(params[:id])

It's clear, easy to implement, and easier to use.

Why You Care

Now that I got a 2.0 out the door and dealt with all the major issues, I want to work on a 3.0. And the major change will be completely removing .decorate, all the tests and code to support it, and the hooks into AR/Mongoid. I'm curious about creating a sister repo/gem, maybe draper-infer, which extracts all this functionality out. If you're deadset on using it and interesting in trying to squash the ongoing bugs, then you can do that. But I don't think it's worth it.

Along the way we can continue rolling 2.X versions that support decorate. If 3.0 comes together then let's try to make a 2.X with deprecation warnings to ease the transition.

In the end I hope to have a library that's (a) easy to use, (b) easier to work on, and (c) more resistant to bugs.

[jcasimir]

I should have made some fake issues so this would be #666.

[fluxusfrequency]

LMK if you want any help, Jeff.

[emaiax]

@jcasimir the new syntax is very sweet and similar to AR, but what is going to happen with decorate_collection?

[jdickey]

(pumping fist in air) Oh, yes, thank you! I agree that "explicit over implicit" is The Way To Go™; getting there on my active projects has kept me gainfully employed for the better part of a year now.

I agree that both of your proposed replacements for decorate are major improvements; I'd humbly ask that the first be implemented as well as the second. Yes, ArticleDecorator.find(params[:id]) is concise and clear, once you understand the basics. But I could show ArticleDecorator.new( Article.find(params[:id]) to a quasitechnical manager/client and have a far higher likelihood of him nodding his head without either eyes glazing over or "what's this?"

Writing sweet code: valuable. Writing sweet code a Secondary 2 student could figure out (and therefore said manager has a fighting chance): priceless.

[firedev]

I tried to raise similar concerns here https://github.com/drapergem/draper/issues/636

[himdel]

Yeah, no, what happens when you need to decorate an already existing record?

Suddenly @record.decorate.icon becomes something like "#{@record.class}Decorator".constantize.new?(@record).icon.

To me, being able to call decorate on ActiveRecord instances without actually knowing or caring what they are is the single useful thing about decorators, so please let's keep that working :).

[franzliedke]

Is there any plan to still go forward with this issue?

@himdel How often are you dealing with ActiveRecord objects and don't know their type? In any case, you can still easily add this method to your models' base class, right?

[himdel]

@franzliedke Pretty much every single time :) The model goes through our reporting engine to generate reports, and then you decorate it to get the icons to use.

EDIT: that said, we don't use Draper anymore, replaced by https://github.com/ManageIQ/manageiq/blob/master/app/models/application_record.rb#L18 https://github.com/ManageIQ/manageiq-ui-classic/blob/master/lib/miq_decorator.rb

[codebycliff]

@franzliedke Yeah, we have a couple scenarios where we don't know the type as well. Examples are:

1. With a polymorphic associations you might call decorate on the association without knowing the type.

2. With inheritance (more specifically STI). You have your base class's decorator define the interface and provide sane defaults. Your subclasses then can have a decorator that overrides a specific method to provide different behavior.

That being said, when I last talked with @jcasimir about taking over maintenance, it seemed like he was still hoping this issue would gain some traction. We discussed extracting it out into a contrib package of sorts. That way you can still opt-in if you need the behavior, but by doing so, you understand the risks/issues that the inference may come with. Unfortunately, I currently don't have time to lead up this effort, but I would be happy to move forward with it other people want to take a whack at it. Let me know if and how you want to proceed.