Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Array support #298

Closed
wants to merge 3 commits into from

3 participants

@barelyknown

Added support for arrays. The decorate method returns the original array with its members decorated for any that respond to the decorator_class method.

  Log.all.decorate
@steveklabnik
Owner

Why not a DecoratedCollectionProxy?

@barelyknown

That would be ok, but what if the collection has mixed types? It looks like DecorateCollectionProxy assumes that all of the members will be of the same class.

@steveklabnik
Owner

I'd rather update the DecoratedCollectionProxy to support that case, then. I try not to monkeypatch core types unless I absolutely have to. :)

@barelyknown

Why not just call decorate on every instance then (within a DecoratedCollectionProxy) instead of using the discern_class_from_my_class to set a single class from them all? Am I missing a benefit there? Just the performance gain for avoiding the decorator_class call on every instance?

@steveklabnik
Owner

I didn't write that code originally; @jcasimir did. So I'm not sure. I'll re-examine the code, but let's explore the possibility.

@barelyknown

I like your idea. But, I am worried that people may be counting on a DecoratedCollectionProxy to have a single klass since a good bit of that is exposed in the public interface. But I'm new to draper and to open source rails contributions, so I'm really interested in your thoughts.

@steveklabnik
Owner

Well, two things:

I would consider anyone relying on them only being one class to be an antipattern. Collections are not guaranteed to be homogeneous in Ruby.

We are coming up on a 1.0 release soonish. Or at least, forking off for 1.0 soon. So, if you'd feel better about making this change then, we can do that, too. :)

I'm going to close this because the pull isn't going to be accepted, however, we can keep talking about this here if you'd like.

@barelyknown

Sounds good. I'll probably use my fork for now and then work post 1.0 to implement the changes in a way that works for you.

Thanks for what you do to support the project.

@steveklabnik
Owner

Awesome. :)

@kuraga

Sorry but I'm very interesting about cause of introducing DecoratedCollectionProxy...

@steveklabnik

@kuraga You mean "why did you make DecoratedCollectionProxy?" Because we needed to decorate a collection ;)

@kuraga

@steveklabnik So to decorate Array is just another variant of this job? Or something else was introduced? (As you wrote @jcasimir has to know it.)

@kuraga

By the way I think we should decorate each array item even it's not decoratable. Because if we use decorate for objects it is so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
1  lib/draper.rb
@@ -9,6 +9,7 @@
require 'draper/helper_support'
require 'draper/view_context'
require 'draper/decorated_enumerable_proxy'
+require 'draper/array_support'
require 'draper/railtie' if defined?(Rails)
# Test Support
View
7 lib/draper/array_support.rb
@@ -0,0 +1,7 @@
+module Draper::ArraySupport
+
+ def decorate
+ collect { |item| item.respond_to?(:decorator_class) ? item.decorate : item }
+ end
+
+end
View
4 lib/draper/railtie.rb
@@ -39,6 +39,10 @@ class Railtie < Rails::Railtie
self.send(:include, Draper::ModelSupport)
end
end
+
+ initializer "draper.extend_array" do |app|
+ Array.send(:include, Draper::ArraySupport)
+ end
console do
require 'action_controller/test_case'
Something went wrong with that request. Please try again.