ApplicationDecorator generator #796

chuck-john · 2017-04-04T17:31:58Z

Description

This branch introduces an installation generator. Running rails generate draper:install will create an ApplicationDecorator class, if none already exists. The purpose of this change is encourage use of the Rails' Application naming convention by creating a parent decorator class from which others extend. Due to existing work, generated decorators will inherit from ApplicationDecorator, if present. More information on this feature request can be found here.

Testing

Run rails generate draper:install in the dummy app.
Ensure the class ApplicationDecorator is created in app/decorators/application_decorator.rb.
Ensure newly-generated decorators extend from ApplicationDecorator when present.

To-Dos

N/A

References

GitHub Issue 727

codebycliff

Thanks for this. I think it will be a nice addition.

I had a few minor comments, but we should be good to merge after that.

codebycliff · 2017-04-05T02:13:21Z

spec/generators/install/install_generator_spec.rb

+    subject { file('app/decorators/application_decorator.rb') }
+
+    describe 'naming' do
+      before { run_generator %w(YourModels) }


I don't think you need the %w(YourModels) here since this generator doesn't take any arguments.

codebycliff · 2017-04-05T02:14:56Z

lib/generators/draper/templates/application_decorator.rb

+  #   def percent_amount
+  #     h.number_to_percentage object.amount, precision: 2
+  #   end
+


Remove empty line.

@codebycliff, I was basing this template off of the decorator template. Do you want the two to match?

codebycliff · 2017-04-05T02:24:12Z

README.md

@@ -155,6 +162,8 @@ rails generate decorator Article

 ...to create the `ArticleDecorator`.

+Generated decorators will inherit from `Draper::Decorator` unless an `ApplicationDecorator` exists.


I think it might be more in context to move this up with the rest of the new documentation. Something like:

To create an ApplicationController that all generated decorators inherit from, run:

rails generate draper:install

I don't think the additional comment about them inheriting from Draper::Decorator otherwise is necessary given that the paragraph above it says that all decorators inherit from Draper::Decorator.

I could also see this documentation living in the "Installation" section of the README at some point. I think we should leave it here for now, but just something to think about.

syguer · 2017-04-05T02:44:40Z

spec/generators/install/install_generator_spec.rb

+  describe 'the application decorator' do
+    subject { file('app/decorators/application_decorator.rb') }
+
+    describe 'naming' do


I think we don't need this block, do it?

chuck-john · 2017-04-05T16:56:46Z

lib/generators/draper/templates/application_decorator.rb

@@ -0,0 +1,8 @@
+class ApplicationDecorator < Draper::Decorator


I removed delegate_all from this template. The belief is that certain issues and bad practices may stem from having this line in all decorators by default. Implementers should be encouraged to whitelist their delegations to minimize this risk.

I agree completely. It's best not to assume functionality at the application level. I'd rather the user add this line themselves that way they are forced to understand what it does.

@chuck-john @codebycliff
I agree your opinion too :)
However, how do you think about leaving as comment to notice function of delegate_all? And additionally, if you agree with me, I want to add include Draper::LazyHelpers as comment too.

That's an interesting idea. If we left in any commented out stuff, I'd want to include all draper functionality that can be used (e.g. decorates_finders) with documentation explaining. I think I'd like to wait for this though for the time being unless you feel strongly about it.

codebycliff

I'm good with this. I'll go ahead and merge it unless anyone else has any comments.

syguer · 2017-04-05T17:30:46Z

I've added a bit comment.
But great job. Patch looks good to me 👍

* Release 3.0.0 * Add breaking change #768 to changelog. * Add recently merged changes to the changelog. * Add configuration and custom default controller to the changelog. * Add entry to changelog for pull request 720 * Add entry to changelog for Rails 5 API-only support. * Add changelog entry for pull request #795. * Add changelog entry for pull request #796. * Add changelog entry for enumerable optimization. * Update maintainer information in README. * Add pull request #799 to changelog. * Add compatability documentation to readme.

C.J. Howard and others added 13 commits March 31, 2017 15:16

un-skip test and remove unused class definition

b66e34c

fix build

180ee28

Merge branch 'draper/master'

0dbf6de

Merge branch 'draper/master'

d25d42e

Merge branch 'draper/master'

b48361c

add InstallGenerator

21d7035

re-namespace generators

b89e781

Merge branch 'draper/master'

a556bbb

Merge branch 'master' into feature/install-generator

61be5a0

remove module namespacing

11589fd

update README with new generator command

41abfe9

Merge branch 'draper/master'

d6a3f87

Merge branch 'master' into feature/install-generator

7614c9b

codebycliff added has issue labels Apr 4, 2017

C.J. Howard added 9 commits April 4, 2017 15:42

inherit from Base instead of NamedBase

e16bf4d

add inheritance line to Generators section of README

06b2527

call #copy_file instead of #template

ef6eb61

remove skip functionality

e7cbc49

refactor once-used variable

4b145b4

specify application_decorator.rb file instead of class

7b21a6d

move decorator generator back to rails namespace

f0d03ad

revert README change

fa6c9ce

specify decorator class

1f165e6

chuck-john changed the title ~~installation generator~~ ApplicationDecorator generator Apr 5, 2017

re-word template help text

bd76ae6

codebycliff reviewed Apr 5, 2017

View reviewed changes

codebycliff added awaiting response and removed work in progress labels Apr 5, 2017

syguer reviewed Apr 5, 2017

View reviewed changes

C.J. Howard added 3 commits April 5, 2017 11:50

remove unnecessary describe block

161717e

remove delegate_all from application_decorator template

511993f

edit README

6196073

chuck-john commented Apr 5, 2017

View reviewed changes

codebycliff removed the awaiting response label Apr 5, 2017

codebycliff approved these changes Apr 5, 2017

View reviewed changes

codebycliff merged commit a1d854a into drapergem:master Apr 5, 2017

codebycliff mentioned this pull request Apr 5, 2017

ApplicationDecorator class #727

Closed

codebycliff added a commit to codebycliff/draper that referenced this pull request Apr 5, 2017

Add changelog entry for pull request drapergem#796.

3a69921

codebycliff added this to the 3.0.0 milestone Apr 5, 2017

chuck-john deleted the feature/install-generator branch April 6, 2017 23:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ApplicationDecorator generator #796

ApplicationDecorator generator #796

chuck-john commented Apr 4, 2017 •

edited

Loading

codebycliff left a comment

codebycliff Apr 5, 2017

codebycliff Apr 5, 2017

chuck-john Apr 5, 2017

codebycliff Apr 5, 2017

syguer Apr 5, 2017 •

edited

Loading

chuck-john Apr 5, 2017

codebycliff Apr 5, 2017

syguer Apr 5, 2017 •

edited

Loading

codebycliff Apr 5, 2017

codebycliff left a comment

syguer commented Apr 5, 2017

		@@ -155,6 +162,8 @@ rails generate decorator Article

		...to create the `ArticleDecorator`.

		Generated decorators will inherit from `Draper::Decorator` unless an `ApplicationDecorator` exists.

		@@ -0,0 +1,8 @@
		class ApplicationDecorator < Draper::Decorator

ApplicationDecorator generator #796

ApplicationDecorator generator #796

Conversation

chuck-john commented Apr 4, 2017 • edited Loading

Description

Testing

To-Dos

References

codebycliff left a comment

Choose a reason for hiding this comment

codebycliff Apr 5, 2017

Choose a reason for hiding this comment

codebycliff Apr 5, 2017

Choose a reason for hiding this comment

chuck-john Apr 5, 2017

Choose a reason for hiding this comment

codebycliff Apr 5, 2017

Choose a reason for hiding this comment

syguer Apr 5, 2017 • edited Loading

Choose a reason for hiding this comment

chuck-john Apr 5, 2017

Choose a reason for hiding this comment

codebycliff Apr 5, 2017

Choose a reason for hiding this comment

syguer Apr 5, 2017 • edited Loading

Choose a reason for hiding this comment

codebycliff Apr 5, 2017

Choose a reason for hiding this comment

codebycliff left a comment

Choose a reason for hiding this comment

syguer commented Apr 5, 2017

chuck-john commented Apr 4, 2017 •

edited

Loading

syguer Apr 5, 2017 •

edited

Loading

syguer Apr 5, 2017 •

edited

Loading