Add option to specify namespace for decorator lookup #480

Merged
merged 1 commit into from Feb 19, 2013

Conversation

Projects
None yet
4 participants
@urbanautomaton
Contributor

urbanautomaton commented Feb 19, 2013

Hi,

In our app, we use decorators in a number of different contexts - e.g. constructing an API response, rendering HTML templates, and so forth. We keep separate decorators for each context, and namespace them to keep them organised, so our Activity model has a root-level decorator, but also API::ActivityDecorator and HTML::ActivityDecorator.

This patch adds a :namespace option to the various decoration methods, so that users can do things like this:

> Activity.last.decorate
=> #<ActivityDecorator:0x000001030d0ad0
  @context={},
  @source=#<Activity id:1>>
> Activity.last.decorate(:namespace => HTML)
=> #<HTML::ActivityDecorator:0x000001030d0ad0
  @context={},
  @source=#<Activity id:1>>

The option can also be passed to Decorator.decorate_collection, .decorates_association and the like:

module API
  class ActivityDecorator < Draper::Decorator
    decorates_association :comments, :namespace => API
  end
end
> Activity.last.decorate(:namespace => API).comments.first
=> #<API::CommentDecorator:0x000001030d0ad0
  @context={},
  @source=#<Comment id:1>>

Is this a feature you'd be interested in including? The ability to organise our decorators in this manner has certainly helped us a lot, and we think others might find it useful too...

Cheers,
Simon

Simon Coffey
Add option to specify namespace for decorator lookup
To use different decorators in different contexts (e.g. HTML rendering
vs. API representation), the ability to specify a decorator namespace is
added. For example:

    > Product.new.decorate
    => #<ProductDecorator:0x0000010b6e47e8>
    > Product.new.decorate(:namespace => HTML)
    => #<HTML::ProductDecorator:0x0000010b6e47e9>
@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Feb 19, 2013

Member

Hmm. I am wary to write code that messes with Ruby's normal lookup semantics.

Member

steveklabnik commented Feb 19, 2013

Hmm. I am wary to write code that messes with Ruby's normal lookup semantics.

@urbanautomaton

This comment has been minimized.

Show comment
Hide comment
@urbanautomaton

urbanautomaton Feb 19, 2013

Contributor

Are we doing that? It adjusts how the constant name that's eventually looked up is put together, but I don't think we're changing the actual lookup semantics...

Edit: put another way, is prepending "SomeNamespace::" to a class name any different to appending "Decorator" to it?

Contributor

urbanautomaton commented Feb 19, 2013

Are we doing that? It adjusts how the constant name that's eventually looked up is put together, but I don't think we're changing the actual lookup semantics...

Edit: put another way, is prepending "SomeNamespace::" to a class name any different to appending "Decorator" to it?

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Feb 19, 2013

Member

You're right: I caught this PR before Travis was done building, so I didn't even look at your implementation. ;)

Seems fine. Let's do it!

Member

steveklabnik commented Feb 19, 2013

You're right: I caught this PR before Travis was done building, so I didn't even look at your implementation. ;)

Seems fine. Let's do it!

steveklabnik added a commit that referenced this pull request Feb 19, 2013

Merge pull request #480 from urbanautomaton/decorator-namespace-option
Add option to specify namespace for decorator lookup

@steveklabnik steveklabnik merged commit f93542c into drapergem:master Feb 19, 2013

1 check passed

default The Travis build passed
Details
@urbanautomaton

This comment has been minimized.

Show comment
Hide comment
@urbanautomaton

urbanautomaton Feb 20, 2013

Contributor

Nice one, thanks very much. :-)

Contributor

urbanautomaton commented Feb 20, 2013

Nice one, thanks very much. :-)

artempartos added a commit to artempartos/draper that referenced this pull request Mar 10, 2013

Revert "Merge pull request #480 from urbanautomaton/decorator-namespa…
…ce-option"

This reverts commit f93542c, reversing
changes made to f4dd9e0.
@kryzhovnik

This comment has been minimized.

Show comment
Hide comment
@kryzhovnik

kryzhovnik Mar 12, 2013

Why revert? Is this feature was rejected?

I was going to use it to separate "admin area" (Admin::ProductDecorator) decorators and public one (ProductDecorator). I think it is very useful feature.

Why revert? Is this feature was rejected?

I was going to use it to separate "admin area" (Admin::ProductDecorator) decorators and public one (ProductDecorator). I think it is very useful feature.

@haines

This comment has been minimized.

Show comment
Hide comment
@haines

haines Mar 13, 2013

Contributor

@kryzhovnik Nope, as I noted on #494, I really like the feature too, but I think we need to find a way to do this without propagating the namespace option throughout the codebase.

Contributor

haines commented Mar 13, 2013

@kryzhovnik Nope, as I noted on #494, I really like the feature too, but I think we need to find a way to do this without propagating the namespace option throughout the codebase.

urbanautomaton pushed a commit to urbanautomaton/draper that referenced this pull request Mar 13, 2013

Simon Coffey
Add option to specify namespace for decorator lookup
To use different decorators in different contexts (e.g. HTML rendering
vs. API representation), the ability to specify a decorator namespace is
added. For example:

    > Product.new.decorate
    => #<ProductDecorator:0x0000010b6e47e8>
    > Product.new.decorate(:namespace => HTML)
    => #<HTML::ProductDecorator:0x0000010b6e47e9>

Second attempt, following #480 and #494.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment