Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

.decorate is the Devil #661

Open
jcasimir opened this issue Mar 27, 2015 · 9 comments
Open

.decorate is the Devil #661

jcasimir opened this issue Mar 27, 2015 · 9 comments

Comments

@jcasimir
Copy link
Member

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
Copy link
Member Author

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

@fluxusfrequency
Copy link

LMK if you want any help, Jeff.

@emaiax
Copy link

emaiax commented Mar 28, 2015

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

@jdickey
Copy link

jdickey commented Mar 29, 2015

(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
Copy link

firedev commented Apr 23, 2015

I tried to raise similar concerns here #636

@himdel
Copy link

himdel commented Dec 21, 2016

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
Copy link

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
Copy link

himdel commented Jan 16, 2018

@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
Copy link
Collaborator

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants