Skip to content

Loading…

Decorated collection also responds to decorated_with? #500

Merged
merged 2 commits into from

3 participants

@drueck

I just started using Draper yesterday (I really like it so far), and I was writing a controller spec to make sure that my collection was decorated with my custom CollectionDecorator which added support for will_paginate, and I was surprised that the decorated collection did not respond to decorated_with? like the regular Decorator does.

I took a look at the code and I see that you would probably use instance_of? to test this because it looks like a collection is only decorated by one CollectionDecorator (not many), but it still might be nice to have the same interface for both Decorators and CollectionDecorators for this method.

There very well might be a good reason you guys chose not to do this, though, so feel free to ignore this pull request if so.

Thanks!

@steveklabnik steveklabnik commented on an outdated diff
lib/draper/collection_decorator.rb
@@ -66,6 +66,13 @@ def decorated?
true
end
+ # Checks if a given decorator has been applied to the collection.
+ #
+ # @param [Class] decorator_class
+ def decorated_with?(decorator_class)
+ self.instance_of? decorator_class
+ end
@steveklabnik drapergem member

I think I'd rather just have this as alias_method :kind_of, :decorated_with or whatever the aliasing syntax is ;)

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

I'm pretty sure this is just an oversight in the API. @haines what do you think?

@drueck

Something I didn't think of until I after I submitted this is that as of now I don't think the decorated_with? method is being added to ActiveRecord::Relation, so if you called it on an undecorated collection it would just throw a NoMethodError. That wouldn't be the behavior you would want.... and my patch doesn't address that. Perhaps that is part of the reason why it wasn't included already?

@haines
drapergem member

@drueck You're right, that is the reason why it wasn't included originally; but neither was decorated? until we had a PR (#447, #460) and decided that it was still worthwhile because your tests will fail when you use the method on an undecorated collection (albeit with a less helpful message).

So I do think we should add it, although I agree with @steveklabnik that it would be better just to have

alias_method :decorated_with?, :instance_of?

(instance_of? rather than kind_of?, I think, because that is how Decorator behaves)

If you wouldn't mind making that little change, we'll merge this in! :heart:

@drueck

Cool, thanks!

@steveklabnik steveklabnik merged commit dc430b3 into drapergem:master

1 check passed

Details default The Travis build passed
@steveklabnik
drapergem member

Thanks! :heart:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 13, 2013
  1. @drueck
Commits on Mar 14, 2013
  1. @drueck
This page is out of date. Refresh to see the latest.
Showing with 11 additions and 0 deletions.
  1. +2 −0 lib/draper/collection_decorator.rb
  2. +9 −0 spec/draper/collection_decorator_spec.rb
View
2 lib/draper/collection_decorator.rb
@@ -66,6 +66,8 @@ def decorated?
true
end
+ alias_method :decorated_with?, :instance_of?
+
def kind_of?(klass)
decorated_collection.kind_of?(klass) || super
end
View
9 spec/draper/collection_decorator_spec.rb
@@ -243,6 +243,15 @@ module Draper
end
end
+ describe '#decorated_with?' do
+ it "checks if a decorator has been applied to a collection" do
+ decorator = ProductsDecorator.new([Product.new])
+
+ expect(decorator).to be_decorated_with ProductsDecorator
+ expect(decorator).not_to be_decorated_with OtherDecorator
+ end
+ end
+
describe '#kind_of?' do
it 'asks the kind of its decorated collection' do
decorator = ProductsDecorator.new([])
Something went wrong with that request. Please try again.