Config engine to load decorators #260

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
6 participants
Contributor

kunalchaudhari commented Jul 12, 2012

While extending forem classes we need to require decorators in main app's application.rb. Now with this code they will automatically required when engine get loaded.

This pull request fails (merged 0e8bbb6 into e8fc464).

Contributor

parndt commented Jul 12, 2012

I don't agree. I think we should have app/decorators instead

Contributor

kunalchaudhari commented Jul 13, 2012

I just followed convention. For example model lives under app/models so is it's decorator. I think this will be easy to follow.

Contributor

parndt commented Jul 13, 2012

My reasoning:

  • Models live under app/models
  • Controllers live under app/controllers
  • Views live under app/views
  • Decorators live under app/decorators
  • Presenters live under app/presenters

@radar I think Spree uses the conventions suggested by Kunal -- was this always the case?

Refinery has been using app/decorators for a while.. I guess we didn't get the memo to synchronise.

Collaborator

radar commented Jul 13, 2012

With Spree we have decorators inside their relative directories. I can see where @parndt is coming from and agree with it, but only if the structure is like this:

app/decorators
  |-> models
        |-> post_decorator.rb
  |-> controllers
        |-> posts_controller_decorator.rb
Contributor

parndt commented Jul 13, 2012

Yes that is exactly how we do it in Refinery.

Contributor

parndt commented Jul 13, 2012

We can get around this entire debate if we load app/*_/__decorator.rb but I'm not sure about that.

Collaborator

radar commented Jul 13, 2012

That's what @kunalchaudhari's initial patch does, but I would rather all the decorators not be mixed in with real controllers/models/whatever.

Contributor

parndt commented Jul 13, 2012

Right you are -- that is what it does.. I'm mainly referring to the guide that comes with the patch.

Collaborator

radar commented Jul 13, 2012

The guide should be patched to contain correct namespacing of the overrides as well, i.e.

app/decorators/controllers/forem/posts_controller_decorator.rb

Rather than:

app/decorators/posts_controller_decorator.rb
Contributor

parndt commented Jul 13, 2012

100% agree.

Collaborator

radar commented Jul 13, 2012

🤘

Contributor

kunalchaudhari commented Jul 13, 2012

@radar missed the namespace in readme. actually had followed that in my project. Sorry for that. so we are going with spree like decorator and all decorators are namespaced?

Collaborator

radar commented Jul 13, 2012

That's right. Exactly as I've explained above.

Contributor

parndt commented Jul 13, 2012

Correct so decorators live under app/decorators/

This pull request passes (merged 98c664c into e8fc464).

README.md
+Standard practice for including such changes in your application or extension is to create a file within the relevant app/models or app/controllers directory with the original class name with _decorator appended.
+
+### Adding a custom method to the Post model:
+ # app/models/forem/post_decorator.rb
@parndt

parndt Jul 13, 2012

Contributor

Please make this app/decorators/models/forem/post_decorator.rb

README.md
+ end
+
+### Adding a custom method to the PostsController:
+ # app/controllers/forem/posts_controller_decorator.rb
@parndt

parndt Jul 13, 2012

Contributor

Please make this app/decorators/controllers/forem/posts_controller_decorator.rb

This pull request passes (merged 47cabe2 into e8fc464).

Contributor

parndt commented Jul 13, 2012

Thank you very much -- I've merged this into 646c3e0 :-)

@parndt parndt closed this Jul 13, 2012

+ def self.activate
+
+ Dir.glob(File.join(File.dirname(__FILE__), "../app/**/*_decorator*.rb")) do |c|
+ Rails.configuration.cache_classes ? require(c) : load(c)
@neerajdotname

neerajdotname Jul 13, 2012

Just curious . Any particular reason why this require vs load is being handled manually. Is Rails not equipped to unload these constants ?

@radar

radar Jul 16, 2012

Collaborator

Legacy from Spree. I think you're right. We could probably get away with using require_dependency here?

radar added a commit that referenced this pull request Jul 20, 2012

Fix decorator loading code to:
- Only load decorators in app/decorators
- Actually load the decorators
- Find decorators at Rails.root rather than ugly file path

Related to #260 and fixes #266

atd commented Oct 6, 2012

FYI: I packaged this functionality as a gem, so it can be used by other engines. I also mentioned you as authors.

https://github.com/atd/rails_engine_decorators

gonzalo-bulnes referenced this pull request in gonzalo-bulnes/simple_token_authentication Mar 16, 2014

Add illustrative scenario for Rails Engine Decorators issue
Run `cucumber features/smoke_decorators.feature` to see it.

A (very) verbose output can be activated by uncommenting
the @announce tag of one or both scenarii.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment