Mention Draper::Decoratable in docs #371

Closed
tovodeverett opened this Issue Dec 9, 2012 · 3 comments

Comments

Projects
None yet
3 participants
Contributor

tovodeverett commented Dec 9, 2012

I have a couple of model classes that don't inherit from ActiveRecord. In Draper 0.18.0 I could create a Decorator class and use decorates: model and everything just worked. In the current git version, the Decorator worked fine (once I dealt with the fact that I'm using a plural model name for "Prefs" by declaring the name as uncountable in config/initializers/inflections.rb), but I couldn't call #decorate on the model.

After a little exploration I simply added include Draper::Decoratable to my model class and that resolved the issues. This could either be handled by documenting the solution or by adding a Draper::Decorator.inherited method that automagically includes Draper::Decoratable into the source class if the source class doesn't already respond to decorate.

The latter approach may have some potential sticky issues with Draper::Decorator.decorates - from my experimentation, it appears that inherited gets called before the code in the block to setup the subclass gets called. As a result, inherited will get called before the user has a chance to call decorates if the decorator doesn't match the name of the model. So in this situation, the code would need to silently fail and then somehow get retriggered after any call to decorates.

I'd be happy to try my hand at coding up a solution, but before I go to work it would be nice to know whether this should be handled in documentation or code.

Also, I'd like to start a list of ideas for things that should go in the documentation (i.e. best practices, advice, suggestions on how to test Decorators with RSpec, etc.). Once I get to a point where I'm using Draper correctly, I'd be happy to work on the documentation. Should I create a separate issue for each documentation idea, or can I lump them all in one issue?

Owner

steveklabnik commented Dec 9, 2012

This could either be handled by documenting the solution or by adding a Draper::Decorator.inherited method that automagically includes Draper::Decoratable into the source class if the source class doesn't already respond to decorate.

I vote #2. I'm not 100% sure that I fully grasp your sticky issue, though: I don't subclass, ever, so it's not something I've tried. Hm.

Should I create a separate issue for each documentation idea, or can I lump them all in one issue?

Once all these bugs are worked out, I want to re-write all the docs, then release 1.0. So just chill till after that commit lands, I think. Make sense?

Contributor

tovodeverett commented Dec 9, 2012

The sticky issue is this:

class NotTheModelNameDecorator < Draper::Decorator
  decorates ActualModel
end

Based on my explorations, I believe the inherited call will get made on Draper::Decorator before the call to decorates. I did have to test this, because I could easily see the behavior being that inherited gets called after the block completes rather than before. I consulted a number of books (Pickaxe, Metaprogramming Ruby, Eloquent Ruby, and The Ruby Programming Language) and none of them seemed to explicitly state whether the inherited method got called before or after the block, which is why I had to go experiment. Here's a demonstration:

irb(main):001:0> class Foo
irb(main):002:1>   def self.inherited(subklass)
irb(main):003:2>     puts "Inherited #{subklass} - #{subklass.desc}"
irb(main):004:2>   end
irb(main):005:1>   def self.desc
irb(main):006:2>     "This is class Foo"
irb(main):007:2>   end
irb(main):008:1> end
=> nil
irb(main):009:0> class Bar < Foo
irb(main):010:1>   def self.desc
irb(main):011:2>     "Bar is a subclass of Foo"
irb(main):012:2>   end
irb(main):013:1> end
Inherited Bar - This is class Foo
=> nil
irb(main):014:0> Bar.desc
=> "Bar is a subclass of Foo"

If inherited got called after the block, we would expect the result from 13.1 to be "Inherited Bar - Bar is a subclass of Foo", but instead we get Foo.desc instead of Bar.desc because Bar.desc has yet to be defined.

As a result, when inherited goes to determine the source class using source_class (so it knows into which class to include Draper::Decoratable), source_class will call inferred_source_class, which will in turn call uninferrable_source. Kaboom! So inherited needs to rescue Draper::UninferrableSourceError in order to permit the class to come into existence (although it won't make the source class decoratable yet). Then when decorates gets called, in addition to setting @source_class, it needs to call inherited again so inherited has a chance to attempt to include Draper::Decoratable on the real source class. This latter call shouldn't rescue Draper::UninferrableSourceError (although it shouldn't need to, because you would have a hard time calling decorates with a Class that doesn't exist).

I'm not suggesting I write the docs yet, just that every time I run into an issue as a new Draper user, I stash a note saying, "Address this issue somewhere in the docs". Right now I have fairly fresh memories of all the mistakes and issues I'm running into, but two months from now I won't remember what I did wrong! I'll go ahead and do it on my end if you'd prefer - I'm just thinking it will be helpful if I have records of what I would have wanted to know.

Owner

steveklabnik commented Dec 9, 2012

Ahhh yes, that does make sense. inherited should get called first. Maybe, for now, we should just solve this through documentation, and make people include the module themselves.

stash a note saying, "Address this issue somewhere in the docs".

That's awesome. ❤️

haines closed this in 52360b6 Dec 31, 2012

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