Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Simplify decorator inferral #394

Merged
merged 3 commits into from

3 participants

@haines
Collaborator

I removed the with: :infer option from collection decorators, and tidied up DecoratedAssociation correspondingly. If :with is not supplied, the item decorator is inferred from the collection decorator (ProductsDecorator => ProductDecorator). If that fails, then decorate is called on each individual item when decorating the collection.

This fixes an issue reported here and here.

I also removed the possibility of changing the decorator's source after instantiation. For collection decorators this wouldn't have worked at all due to the caching, and for singular decorators I think it's probably a bad idea (although it would have worked).

@steveklabnik

Fails on rbx :/

@haines
Collaborator

Weird! It worked on the second build (although that failed on JRuby because the worker died), and my repo's build which passed.

I tried it a few times locally, it seems to be a spec order issue - it works for me with seed 48184 but not for 10772. Yuk! I will investigate.

@haines
Collaborator

Ok, found the problem. The spec that breaks it does this:

Draper::CollectionDecorator.should_receive(:decorate)

And, surprisingly, RSpec doesn't tear this down correctly - after that spec, CollectionDecorator no longer responds to decorate. It turns out that it only happens for aliases of new... any other class method and this would work fine.

I've filed a bug: rspec/rspec-mocks#206

@alindeman

Very weird indeed. I'll dig into the rspec issue soon.

@haines haines Update RSpec dependency
Specs don't work with rspec-mocks 2.12.0 due to an issue with
setting expectations on `.decorate` - rspec/rspec-mocks#206
7628d40
@haines
Collaborator

@steveklabnik This is good to go, if you're happy with the changes!

@steveklabnik

157 additions and 324 deletions.

Of course I'm happy! :p

@steveklabnik steveklabnik merged commit 9f49e1c into drapergem:master
@haines haines deleted the haines:inferred_decorators branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 19, 2012
  1. @haines

    Simplify decorator inferral

    haines authored
  2. @haines

    Make source read-only

    haines authored
Commits on Dec 22, 2012
  1. @haines

    Update RSpec dependency

    haines authored
    Specs don't work with rspec-mocks 2.12.0 due to an issue with
    setting expectations on `.decorate` - rspec/rspec-mocks#206
This page is out of date. Refresh to see the latest.
View
3  draper.gemspec
@@ -22,7 +22,8 @@ Gem::Specification.new do |s|
s.add_development_dependency 'ammeter'
s.add_development_dependency 'rake', '~> 0.9.2'
- s.add_development_dependency 'rspec', '~> 2.10'
+ s.add_development_dependency 'rspec', '~> 2.12'
+ s.add_development_dependency 'rspec-mocks', '>= 2.12.1'
s.add_development_dependency 'yard'
s.add_development_dependency 'minitest-rails', '~> 0.2'
s.add_development_dependency 'minitest', '~> 3.0' if RUBY_PLATFORM == "java"
View
33 lib/draper/collection_decorator.rb
@@ -3,20 +3,21 @@ class CollectionDecorator
include Enumerable
include ViewHelpers
- attr_accessor :source, :context, :decorator_class
+ attr_reader :source
alias_method :to_source, :source
- delegate :as_json, *(Array.instance_methods - Object.instance_methods), to: :decorated_collection
+ attr_accessor :context
+
+ array_methods = Array.instance_methods - Object.instance_methods
+ delegate :as_json, *array_methods, to: :decorated_collection
# @param source collection to decorate
- # @param [Hash] options (optional)
- # @option options [Class, Symbol] :with the class used to decorate
- # items, or `:infer` to call each item's `decorate` method instead
+ # @option options [Class] :with the class used to decorate items
# @option options [Hash] :context context available to each item's decorator
def initialize(source, options = {})
options.assert_valid_keys(:with, :context)
@source = source
- @decorator_class = options.fetch(:with) { self.class.inferred_decorator_class }
+ @decorator_class = options[:with]
@context = options.fetch(:context, {})
end
@@ -62,14 +63,14 @@ def context=(value)
each {|item| item.context = value } if @decorated_collection
end
+ def decorator_class
+ @decorator_class ||= self.class.inferred_decorator_class
+ end
+
protected
def decorate_item(item)
- if decorator_class == :infer
- item.decorate(context: context)
- else
- decorator_class.decorate(item, context: context)
- end
+ item_decorator.call(item, context: context)
end
def self.inferred_decorator_class
@@ -85,5 +86,15 @@ def self.inferred_decorator_class
def self.decorator_uninferrable
raise Draper::UninferrableDecoratorError.new(self)
end
+
+ private
+
+ def item_decorator
+ @item_decorator ||= begin
+ decorator_class.method(:decorate)
+ rescue Draper::UninferrableDecoratorError
+ ->(item, options) { item.decorate(options) }
+ end
+ end
end
end
View
64 lib/draper/decorated_association.rb
@@ -1,70 +1,70 @@
module Draper
class DecoratedAssociation
- attr_reader :base, :association, :options
+ def initialize(owner, association, options)
+ options.assert_valid_keys(:with, :scope, :context)
- def initialize(base, association, options)
- @base = base
+ @owner = owner
@association = association
- options.assert_valid_keys(:with, :scope, :context)
- @options = options
+
+ @decorator_class = options[:with]
+ @scope = options[:scope]
+ @context = options.fetch(:context, owner.context)
end
def call
return undecorated if undecorated.nil?
- decorate
+ decorated
end
- def source
- base.source
+ def context
+ return @context.call(owner.context) if @context.respond_to?(:call)
+ @context
end
private
+ attr_reader :owner, :association, :decorator_class, :scope
+
+ def source
+ owner.source
+ end
+
def undecorated
@undecorated ||= begin
associated = source.send(association)
- associated = associated.send(options[:scope]) if options[:scope]
+ associated = associated.send(scope) if scope
associated
end
end
- def decorate
- @decorated ||= decorator_class.send(decorate_method, undecorated, decorator_options)
- end
-
- def decorate_method
- if collection? && decorator_class.respond_to?(:decorate_collection)
- :decorate_collection
- else
- :decorate
- end
+ def decorated
+ @decorated ||= decorator.call(undecorated, context: context)
end
def collection?
undecorated.respond_to?(:first)
end
- def decorator_class
- return options[:with] if options[:with]
+ def decorator
+ return collection_decorator if collection?
- if collection?
- options[:with] = :infer
- Draper::CollectionDecorator
+ if decorator_class
+ decorator_class.method(:decorate)
else
- undecorated.decorator_class
+ ->(item, options) { item.decorate(options) }
end
end
- def decorator_options
- decorator_class # Ensures options[:with] = :infer for unspecified collections
+ def collection_decorator
+ klass = decorator_class || Draper::CollectionDecorator
- dec_options = collection? ? options.slice(:with, :context) : options.slice(:context)
- dec_options[:context] = base.context unless dec_options.key?(:context)
- if dec_options[:context].respond_to?(:call)
- dec_options[:context] = dec_options[:context].call(base.context)
+ if klass.respond_to?(:decorate_collection)
+ klass.method(:decorate_collection)
+ else
+ klass.method(:decorate)
end
- dec_options
end
+
end
end
View
14 lib/draper/decorator.rb
@@ -5,11 +5,12 @@ class Decorator
include Draper::ViewHelpers
include ActiveModel::Serialization if defined?(ActiveModel::Serialization)
- attr_accessor :source, :context
-
+ attr_reader :source
alias_method :model, :source
alias_method :to_source, :source
+ attr_accessor :context
+
# Initialize a new decorator instance by passing in
# an instance of the source class. Pass in an optional
# :context inside the options hash which is available
@@ -23,7 +24,6 @@ class Decorator
# multiple places in the chain.
#
# @param [Object] source object to decorate
- # @param [Hash] options (optional)
# @option options [Hash] :context context available to the decorator
def initialize(source, options = {})
options.assert_valid_keys(:context)
@@ -136,8 +136,8 @@ def self.allows(*methods)
# @param [Object] source collection to decorate
# @param [Hash] options passed to each item's decorator (except
# for the keys listed below)
- # @option options [Class,Symbol] :with (self) the class used to decorate
- # items, or `:infer` to call each item's `decorate` method instead
+ # @option options [Class] :with (self) the class used to decorate
+ # items
# @option options [Hash] :context context available to decorated items
def self.decorate_collection(source, options = {})
options.assert_valid_keys(:with, :context)
@@ -258,8 +258,8 @@ def allow?(method)
def handle_multiple_decoration(options)
if source.instance_of?(self.class)
- self.context = source.context unless options.has_key?(:context)
- self.source = source.source
+ @context = source.context unless options.has_key?(:context)
+ @source = source.source
elsif source.decorated_with?(self.class)
warn "Reapplying #{self.class} decorator to target that is already decorated with it. Call stack:\n#{caller(1).join("\n")}"
end
View
52 spec/draper/collection_decorator_spec.rb
@@ -79,52 +79,64 @@
expect { Draper::CollectionDecorator.new(source, valid_options.merge(foo: 'bar')) }.to raise_error(ArgumentError, 'Unknown key: foo')
end
end
+ end
+
+ describe "#source" do
+ it "returns the source collection" do
+ subject.source.should be source
+ end
+
+ it "is aliased to #to_source" do
+ subject.to_source.should be source
+ end
+ end
+
+ describe "item decoration" do
+ subject { subject_class.new(source, options) }
+ let(:decorator_classes) { subject.decorated_collection.map(&:class) }
+ let(:source) { [Product.new, Widget.new] }
+
+ context "when the :with option was given" do
+ let(:options) { {with: SpecificProductDecorator} }
- context "when the :with option is given" do
context "and the decorator can't be inferred from the class" do
- subject { Draper::CollectionDecorator.new(source, with: ProductDecorator) }
+ let(:subject_class) { Draper::CollectionDecorator }
it "uses the :with option" do
- subject.decorator_class.should be ProductDecorator
+ decorator_classes.should == [SpecificProductDecorator, SpecificProductDecorator]
end
end
context "and the decorator is inferrable from the class" do
- subject { ProductsDecorator.new(source, with: SpecificProductDecorator) }
+ let(:subject_class) { ProductsDecorator }
it "uses the :with option" do
- subject.decorator_class.should be SpecificProductDecorator
+ decorator_classes.should == [SpecificProductDecorator, SpecificProductDecorator]
end
end
end
- context "when the :with option is not given" do
+ context "when the :with option was not given" do
+ let(:options) { {} }
+
context "and the decorator can't be inferred from the class" do
- it "raises an UninferrableDecoratorError" do
- expect{Draper::CollectionDecorator.new(source)}.to raise_error Draper::UninferrableDecoratorError
+ let(:subject_class) { Draper::CollectionDecorator }
+
+ it "infers the decorator from each item" do
+ decorator_classes.should == [ProductDecorator, WidgetDecorator]
end
end
context "and the decorator is inferrable from the class" do
- subject { ProductsDecorator.new(source) }
+ let(:subject_class) { ProductsDecorator}
it "infers the decorator" do
- subject.decorator_class.should be ProductDecorator
+ decorator_classes.should == [ProductDecorator, ProductDecorator]
end
end
end
end
- describe "#source" do
- it "returns the source collection" do
- subject.source.should be source
- end
-
- it "is aliased to #to_source" do
- subject.to_source.should be source
- end
- end
-
describe "#find" do
context "with a block" do
it "decorates Enumerable#find" do
View
306 spec/draper/decorated_association_spec.rb
@@ -1,313 +1,131 @@
require 'spec_helper'
describe Draper::DecoratedAssociation do
- let(:decorated_association) { Draper::DecoratedAssociation.new(base, association, options) }
+ let(:decorated_association) { Draper::DecoratedAssociation.new(owner, :association, options) }
let(:source) { Product.new }
- let(:base) { source.decorate }
+ let(:owner) { source.decorate }
let(:options) { {} }
describe "#initialize" do
describe "options validation" do
- let(:association) { :similar_products }
let(:valid_options) { {with: ProductDecorator, scope: :foo, context: {}} }
it "does not raise error on valid options" do
- expect { Draper::DecoratedAssociation.new(base, association, valid_options) }.to_not raise_error
+ expect { Draper::DecoratedAssociation.new(owner, :association, valid_options) }.to_not raise_error
end
it "raises error on invalid options" do
- expect { Draper::DecoratedAssociation.new(base, association, valid_options.merge(foo: 'bar')) }.to raise_error(ArgumentError, 'Unknown key: foo')
+ expect { Draper::DecoratedAssociation.new(owner, :association, valid_options.merge(foo: 'bar')) }.to raise_error(ArgumentError, 'Unknown key: foo')
end
end
end
- describe "#base" do
- subject { decorated_association.base }
- let(:association) { :similar_products }
-
- it "returns the base decorator" do
- should be base
- end
-
- it "returns a Decorator" do
- subject.class.should == ProductDecorator
- end
- end
-
- describe "#source" do
- subject { decorated_association.source }
- let(:association) { :similar_products }
-
- it "returns the base decorator's source" do
- should be base.source
- end
-
- it "returns a Model" do
- subject.class.should == Product
- end
- end
-
describe "#call" do
- subject { decorated_association.call }
-
- context "for an ActiveModel collection association" do
- let(:association) { :similar_products }
-
- context "when the association is not empty" do
- it "decorates the collection" do
- subject.should be_a Draper::CollectionDecorator
- end
-
- it "infers the decorator" do
- subject.decorator_class.should be :infer
- end
- end
+ let(:context) { {foo: "bar"} }
+ let(:expected_options) { {context: context} }
- context "when the association is empty" do
- it "returns an empty collection decorator" do
- source.stub(:similar_products).and_return([])
- subject.should be_a Draper::CollectionDecorator
- subject.should be_empty
- subject.first.should be_nil
- end
- end
+ before do
+ source.stub association: associated
+ decorated_association.stub context: context
end
- context "for non-ActiveModel collection associations" do
- let(:association) { :poro_similar_products }
+ context "for a singular association" do
+ let(:associated) { Product.new }
- context "when the association is not empty" do
- it "decorates the collection" do
- subject.should be_a Draper::CollectionDecorator
- end
-
- it "infers the decorator" do
- subject.decorator_class.should be :infer
- end
- end
-
- context "when the association is empty" do
- it "returns an empty collection decorator" do
- source.stub(:poro_similar_products).and_return([])
- subject.should be_a Draper::CollectionDecorator
- subject.should be_empty
- subject.first.should be_nil
- end
- end
- end
-
- context "for an ActiveModel singular association" do
- let(:association) { :previous_version }
+ context "when :with option was given" do
+ let(:options) { {with: decorator} }
+ let(:decorator) { SpecificProductDecorator }
- context "when the association is present" do
- it "decorates the association" do
- subject.should be_decorated_with ProductDecorator
+ it "uses the specified decorator" do
+ decorator.should_receive(:decorate).with(associated, expected_options).and_return(:decorated)
+ decorated_association.call.should be :decorated
end
end
- context "when the association is absent" do
- it "doesn't decorate the association" do
- source.stub(:previous_version).and_return(nil)
- subject.should be_nil
+ context "when :with option was not given" do
+ it "infers the decorator" do
+ associated.should_receive(:decorate).with(expected_options).and_return(:decorated)
+ decorated_association.call.should be :decorated
end
end
end
- context "for a non-ActiveModel singular association" do
- let(:association) { :poro_previous_version }
+ context "for a collection association" do
+ let(:associated) { [Product.new, Widget.new] }
- context "when the association is present" do
- it "decorates the association" do
- subject.should be_decorated_with ProductDecorator
- end
- end
+ context "when :with option is a collection decorator" do
+ let(:options) { {with: collection_decorator} }
+ let(:collection_decorator) { ProductsDecorator }
- context "when the association is absent" do
- it "doesn't decorate the association" do
- source.stub(:poro_previous_version).and_return(nil)
- subject.should be_nil
+ it "uses the specified decorator" do
+ collection_decorator.should_receive(:decorate).with(associated, expected_options).and_return(:decorated_collection)
+ decorated_association.call.should be :decorated_collection
end
end
- end
-
- context "when a decorator is specified" do
- let(:options) { {with: SpecificProductDecorator} }
- context "for a singular association" do
- let(:association) { :previous_version }
+ context "when :with option is a singular decorator" do
+ let(:options) { {with: decorator} }
+ let(:decorator) { SpecificProductDecorator }
- it "decorates with the specified decorator" do
- subject.should be_decorated_with SpecificProductDecorator
+ it "uses a CollectionDecorator of the specified decorator" do
+ decorator.should_receive(:decorate_collection).with(associated, expected_options).and_return(:decorated_collection)
+ decorated_association.call.should be :decorated_collection
end
end
- context "for a collection association" do
- let(:association) { :similar_products}
-
- it "decorates with a collection of the specifed decorators" do
- subject.should be_a Draper::CollectionDecorator
- subject.decorator_class.should be SpecificProductDecorator
+ context "when :with option was not given" do
+ it "uses a CollectionDecorator of inferred decorators" do
+ Draper::CollectionDecorator.should_receive(:decorate).with(associated, expected_options).and_return(:decorated_collection)
+ decorated_association.call.should be :decorated_collection
end
end
end
- context "when a collection decorator is specified" do
- let(:association) { :similar_products }
- let(:options) { {with: ProductsDecorator} }
-
- it "decorates with the specified decorator" do
- subject.should be_a ProductsDecorator
- end
- end
-
context "with a scope" do
- let(:association) { :thing }
+ let(:associated) { [] }
let(:options) { {scope: :foo} }
it "applies the scope before decoration" do
- scoped = [SomeThing.new]
- SomeThing.any_instance.should_receive(:foo).and_return(scoped)
- subject.source.should be scoped
- end
- end
-
- context "base has context" do
- let(:association) { :similar_products }
- let(:base) { source.decorate(context: {some: 'context'}) }
-
- context "when no context is specified" do
- it "it should inherit context from base" do
- subject.context.should == {some: 'context'}
- end
-
- it "it should share context hash with base" do
- subject.context.should be base.context
- end
- end
-
- context "when static context is specified" do
- let(:options) { {context: {other: 'context'}} }
-
- it "it should get context from static option" do
- subject.context.should == {other: 'context'}
- end
- end
-
- context "when lambda context is specified" do
- let(:options) { {context: lambda {|context| context.merge(other: 'protext')}} }
-
- it "it should get generated context" do
- subject.context.should == {some: 'context', other: 'protext'}
- end
+ scoped = [:scoped]
+ associated.should_receive(:foo).and_return(scoped)
+ decorated_association.call.source.should be scoped
end
end
end
- describe "#decorator_options" do
- subject { decorated_association.send(:decorator_options) }
-
- context "collection association" do
- let(:association) { :similar_products }
+ describe "#context" do
+ before { owner.stub context: :owner_context }
- context "no options" do
- it "should return default options" do
- should == {with: :infer, context: {}}
- end
+ context "when :context option was given" do
+ let(:options) { {context: context} }
- it "should set with: to :infer" do
- decorated_association.send(:options).should == options
- subject
- decorated_association.send(:options).should == {with: :infer}
- end
- end
+ context "and is callable" do
+ let(:context) { ->(*){ :dynamic_context } }
- context "option with: ProductDecorator" do
- let(:options) { {with: ProductDecorator} }
- it "should pass with: from options" do
- should == {with: ProductDecorator, context: {}}
+ it "calls it with the owner's context" do
+ context.should_receive(:call).with(:owner_context)
+ decorated_association.context
end
- end
- context "option scope: :to_a" do
- let(:options) { {scope: :to_a} }
- it "should strip scope: from options" do
- decorated_association.send(:options).should == options
- should == {with: :infer, context: {}}
+ it "returns the lambda's return value" do
+ decorated_association.context.should be :dynamic_context
end
end
- context "base has context" do
- let(:base) { source.decorate(context: {some: 'context'}) }
+ context "and is not callable" do
+ let(:context) { :static_context }
- context "no options" do
- it "should return context from base" do
- should == {with: :infer, context: {some: 'context'}}
- end
- end
-
- context "option context: {other: 'context'}" do
- let(:options) { {context: {other: 'context'}} }
- it "should return specified context" do
- should == {with: :infer, context: {other: 'context'}}
- end
- end
-
- context "option context: lambda" do
- let(:options) { {context: lambda {|context| context.merge(other: 'protext')}} }
- it "should return specified context" do
- should == {with: :infer, context: {some: 'context', other: 'protext'}}
- end
+ it "returns the specified value" do
+ decorated_association.context.should be :static_context
end
end
end
- context "singular association" do
- let(:association) { :previous_version }
-
- context "no options" do
- it "should return default options" do
- should == {context: {}}
- end
- end
-
- context "option with: ProductDecorator" do
- let(:options) { {with: ProductDecorator} }
- it "should strip with: from options" do
- should == {context: {}}
- end
- end
-
- context "option scope: :decorate" do
- let(:options) { {scope: :decorate} }
- it "should strip scope: from options" do
- decorated_association.send(:options).should == options
- should == {context: {}}
- end
- end
-
- context "base has context" do
- let(:base) { source.decorate(context: {some: 'context'}) }
-
- context "no options" do
- it "should return context from base" do
- should == {context: {some: 'context'}}
- end
- end
-
- context "option context: {other: 'context'}" do
- let(:options) { {context: {other: 'context'}} }
- it "should return specified context" do
- should == {context: {other: 'context'}}
- end
- end
-
- context "option context: lambda" do
- let(:options) { {context: lambda {|context| context.merge(other: 'protext')}} }
- it "should return specified context" do
- should == {context: {some: 'context', other: 'protext'}}
- end
- end
+ context "when :context option was not given" do
+ it "returns the owner's context" do
+ decorated_association.context.should be :owner_context
end
end
end
+
end
View
9 spec/draper/decorator_spec.rb
@@ -101,15 +101,6 @@
subject.each {|item| item.should be_a ProductDecorator}
end
- context "when given :with => :infer" do
- subject { ProductDecorator.decorate_collection(source, with: :infer) }
-
- it "infers the item decorators" do
- subject.first.should be_a ProductDecorator
- subject.last.should be_a WidgetDecorator
- end
- end
-
context "with context" do
subject { ProductDecorator.decorate_collection(source, with: :infer, context: {some: 'context'}) }
Something went wrong with that request. Please try again.