Done #63 Include a module instead of inherit from a class #67

Closed
wants to merge 7 commits into from

4 participants

@frootloops

I hope, I understand you correctly :)

@harlow

Hi @topdev overall this is looking good. In Rails 4 the concept of concerns will automatically be aded to the load path app/controllers/concerns. I'm thinking for consistency it would be nice to approach this as a controller concern.

http://37signals.com/svn/posts/3372-put-chubby-models-on-a-diet-with-concerns

I'd prefer to remove the word Mixin from the module name PagesControllerMixin. Would be nice to follow the able naming convention from the examples in DHH's article Authenticatable, Taggable, etc.

How do you feel about include HighVoltage::Pageable? If you have any other naming ideas let me know.

@harlow harlow commented on an outdated diff Mar 6, 2013
lib/high_voltage/pages_controller_mixin.rb
+ private
+
+ def current_page
+ page_finder.find
+ end
+
+ def page_finder
+ page_finder_factory.new(params[:id])
+ end
+
+ def page_finder_factory
+ HighVoltage::PageFinder
+ end
+ end
+ end
+end
@harlow
harlow added a note Mar 6, 2013

Are you using SublimeText or TextMate? This file is missing a \n on the last line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@harlow harlow commented on an outdated diff Mar 6, 2013
lib/high_voltage.rb
@@ -3,6 +3,7 @@
require 'high_voltage/page_finder'
require 'high_voltage/route_drawers/default'
require 'high_voltage/route_drawers/root'
@harlow
harlow added a note Mar 6, 2013

include in alphabetical order

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@harlow harlow commented on an outdated diff Mar 6, 2013
@@ -136,7 +136,9 @@ Override the default route:
Then modify it to subclass from High Voltage, adding whatever you need:
@harlow
harlow added a note Mar 6, 2013

Then modify it to include the High Voltage static page concern:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@harlow harlow commented on an outdated diff Mar 6, 2013
lib/high_voltage/pages_controller_mixin.rb
+ unloadable
+ layout Proc.new { |_| HighVoltage.layout }
+
+ rescue_from ActionView::MissingTemplate do |exception|
+ if exception.message =~ %r{Missing template #{page_finder.content_path}}
+ raise ActionController::RoutingError, "No such page: #{params[:id]}"
+ else
+ raise exception
+ end
+ end
+
+ def show
+ render :template => current_page
+ end
+
+ private
@harlow
harlow added a note Mar 6, 2013

I don't think this private will do anything now that its being mixed in as part of a module. May need to hide the methods here so they are not mistaken as actions.

included do
  hide_action :current_page, :page_finder, :page_finder_factory
end
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@manuelmeurer

This does not work yet for me. HighVoltage::Pageable can not be found when I try to include it in a controller in my app. Maybe app/controllers/concerns/high_voltage/pageable.rb has to be required somewhere?

@harlow

@manuelmeurer I think thats right. We'll need to check if the controller/concerns is in the load path, and if its not manually require the concern (in Rails 4 it will be in the load path, but its not by default in Rails 3.x).

Also the more I look at the name pageable it doesn't feel quite right. Will keep thinking about that too.

@linduxed linduxed commented on an outdated diff May 21, 2013
@@ -1,4 +1,4 @@
-# High Voltage [![Build Status](https://travis-ci.org/thoughtbot/high_voltage.png)](http://travis-ci.org/thoughtbot/high_voltage)
+# High Voltage [![Build Status](https://travis-ci.org/thoughtbot/high_voltage.png)](http://travis-ci.org/thoughtbot/high_voltage) [![Dependency Status](https://gemnasium.com/thoughtbot/high_voltage.png)](https://gemnasium.com/thoughtbot/high_voltage)

The inclusion of Gemnasium is out of the scope of this change.

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

This changeset removes all documentation for the old method of inheriting from HighVoltage::PagesController. Since the functionality isn't removed, information about it should be included in the README.

@linduxed linduxed commented on an outdated diff May 21, 2013
@@ -157,7 +159,9 @@ Custom finding
You can further control the algorithm used to find pages by overriding
the `page_finder_factory` method:
- class PagesController < HighVoltage::PagesController
+ class PagesController < V1::Api::ApplicationController

The V1::Api::ApplicationController looks a bit distracting, since the example is about the overriding of a method. I think that ApplicationController would be more generic, and therefore more readable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@linduxed linduxed and 1 other commented on an outdated diff May 21, 2013
app/controllers/concerns/high_voltage/pageable.rb
@@ -0,0 +1,36 @@
+module HighVoltage
+ module Pageable
+ extend ActiveSupport::Concern
+
+ included do
+ unloadable

What is the reason for using unloadable? I couldn't figure it out.

@harlow
harlow added a note May 21, 2013

This needs to be removed. Its handy in development mode, but causes issues in Rails 4. #70

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@linduxed linduxed commented on an outdated diff May 21, 2013
app/controllers/concerns/high_voltage/pageable.rb
+ unloadable
+ layout Proc.new { |_| HighVoltage.layout }
+
+ rescue_from ActionView::MissingTemplate do |exception|
+ if exception.message =~ %r{Missing template #{page_finder.content_path}}
+ raise ActionController::RoutingError, "No such page: #{params[:id]}"
+ else
+ raise exception
+ end
+ end
+
+ def show
+ render :template => current_page
+ end
+
+ hide_action :current_page, :page_finder, :page_finder_factory

While there isn't a problem with this, I don't think it's relevant to this pull-request. This should probably also be tested if it was incorporated into the code base.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@linduxed linduxed commented on an outdated diff May 21, 2013
lib/high_voltage/engine.rb
@@ -1,4 +1,8 @@
module HighVoltage
class Engine < Rails::Engine
+ initializer "Require concern path" do |app|
+ concern_path = "app/controllers/concerns"

This line has trailing whitespace.

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

@topdev Do you have time to work on the raised issues? Would be great to get this functionality in!

Arsen Gasparyan added some commits Jul 26, 2013
@frootloops frootloops commented on the diff Jul 26, 2013
spec/spec_helper.rb
@@ -17,3 +17,15 @@
config.include RSpec::Matchers
config.mock_with :rspec
end
+
+def concern_reload
+ HighVoltage::PagesController.class_eval do
+ if respond_to?(:caches_action)
+ caches_action :show, if: -> { HighVoltage.action_caching }
+ end
+
+ if respond_to?(:caches_page)
+ caches_page :show, if: -> { HighVoltage.page_caching }
+ end
+ end
+end

I don't know how correctly reload concern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@frootloops frootloops commented on an outdated diff Jul 26, 2013
lib/high_voltage/engine.rb
@@ -1,4 +1,12 @@
module HighVoltage
class Engine < Rails::Engine
+ initializer "Require concern path" do |app|
+ concern_path = "app/controllers/concerns"
+ app.paths.add(concern_path) unless app.paths.keys.include?(concern_path)

Maybe require only my concern, without add to load_path?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@linduxed linduxed commented on an outdated diff Jul 26, 2013
lib/high_voltage/engine.rb
end
end
+
+
+

This extra whitespace could be removed.

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

Oh my vim :)

@linduxed linduxed commented on an outdated diff Jul 26, 2013
@@ -203,8 +206,10 @@ the `page_finder_factory` method:
```ruby
# app/controllers/pages_controller.rb
-class PagesController < HighVoltage::PagesController
- private
+class PagesController < ApplicationController
+ include HighVoltage::Pageable
+
+private

It would be consistent with the rest of the project if private was indented by one level.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@linduxed linduxed commented on an outdated diff Jul 26, 2013
spec/controllers/action_caching_controller_spec.rb
@@ -2,25 +2,22 @@
describe HighVoltage::PagesController, 'action_caching' do
it 'caches the action when action_caching is set to true', enable_caching: true do
- HighVoltage.action_caching = true
-
- load('high_voltage/pages_controller.rb')
- get :show, id: 'exists'
-
- action_was_cached(:exists).should be_true
+ expect {
+ HighVoltage.action_caching = true
+ concern_reload
+ get :show, id: :exists
+ }.to change { action_was_cached(:exists) }

This line is misaligned.

You should use do-end here. From the thoughtbot style guide:

Use {...} for single-line blocks. Use do..end for multi-line blocks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@linduxed linduxed commented on an outdated diff Jul 26, 2013
spec/controllers/action_caching_controller_spec.rb
end
it 'does not cache the action when action_caching is set to false', enable_caching: true do
- HighVoltage.action_caching = false
-
- load('high_voltage/pages_controller.rb')
- get :show, id: 'exists'
-
- action_was_cached(:exists).should be_false
+ expect {
+ HighVoltage.action_caching = false
+ concern_reload
+ get :show, id: :exists
+ }.to_not change { action_was_cached(:exists) }

You should use do-end here. From the thoughtbot style guide:

Use {...} for single-line blocks. Use do..end for multi-line blocks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@linduxed linduxed commented on an outdated diff Jul 26, 2013
before_filter :authenticate
layout :layout_for_page
- protected
- def layout_for_page
- case params[:id]
- when 'home'
- 'home'
- else
- 'application'
- end
- end
+protected

It would be consistent with the rest of the project if protected was indented by one level.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Arsen Gasparyan Code style fix 632ff78
@linduxed linduxed commented on an outdated diff Jul 26, 2013
spec/controllers/action_caching_controller_spec.rb
@@ -2,25 +2,22 @@
describe HighVoltage::PagesController, 'action_caching' do
it 'caches the action when action_caching is set to true', enable_caching: true do
- HighVoltage.action_caching = true
-
- load('high_voltage/pages_controller.rb')
- get :show, id: 'exists'
-
- action_was_cached(:exists).should be_true
+ expect do
+ HighVoltage.action_caching = true
+ concern_reload
+ get :show, id: :exists
+ end.to change { action_was_cached(:exists) }

Unfortunately this line is still not correctly aligned. One space more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Gasparyan Arsen More space c791ade
@manuelmeurer

@harlow What do you think about the state of this PR?
I am eager to use this functionality and would be willing to chip in to get this to a state where you would be willing to merge it in.

@harlow

@manuelmeurer I think we're getting pretty close. Looks like we'll need to update the specs around action-caching and reloading the mixin. I think the rest is looking pretty good.

We'll also need to bump the semantic versioning to make sure we don't break anyones current projects.

@manuelmeurer

@topdev Do you have any more time to work on this? I volunteer to take over and open a new PR to fix the last outstanding issues (with credits to you of course)

@harlow

@manuelmeurer I say go for it. I'll be out of town this weekend but should have a little time early next week if you want to try to help get this over the finnish line I'd love to get this merged in.

@manuelmeurer

I tried getting the specs to run but couldn't find a way to reload the module, no matter what.
What's weird is that since the conditions on caches_action and caches_page are defined in procs, they should be reevaluated on every request, right? This doesn't seem to happen, otherwise load('high_voltage/pages_controller.rb') in the specs in action_caching_controller_spec.rb wouldn't be necessary.

@harlow

thanks for the update @manuelmeurer. Seems like we're pretty close here. I'll take a stab at those tests and see if we can get this PR merged.

@manuelmeurer

Great, would be very interested to know how you solve it if you do! 😄

@harlow

This has been squashed and merged into master. I've also bumped the semver version to 2.0.0 just to be safe.

@harlow harlow closed this Oct 4, 2013
@manuelmeurer

Awesome! Can't find 2.0.0 through Rubyems yet though. https://rubygems.org/gems/high_voltage

@harlow

@manuelmeurer I wanted to run a few of our internal projects off of master for the day to make sure there were no hiccups. Just pushed to Rubygems. Should be good to go.

@manuelmeurer

Great thanks!

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