Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Root routes #50

Closed
wants to merge 2 commits into from

3 participants

@harlow
Collaborator
  • Updated readme for root routes
  • New RootRoute constraint
  • Unit test for constraints
  • Config to set root_routes boolean
lib/high_voltage.rb
((9 lines not shown))
mattr_accessor :content_path
- @@content_path = "pages/"
+ @@content_path = 'pages/'
+
+ mattr_accessor :root_routes

This might be complete overkill, but:

Could HighVoltage.route_drawer refer to an object that knows how to draw routes?

It could default to something like HighVoltage::BasicRouteDrawer. Then HighVoltage.root_routes (or .use_root_routes or similar) could be a class method that swaps out the HighVoltage.route_drawer to HighVoltage::RootRouteDrawer?

Then in routes.rb, you could do something like HighVoltage.route_drawer.draw and replace the conditional with polymorphism.

@harlow Collaborator
harlow added a note

I like it. Will put something together for review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
README.md
((20 lines not shown))
You can route the root url to a high voltage page like this:
root :to => 'high_voltage/pages#show', :id => 'home'
Which will render a homepage from app/views/pages/home.html.erb
+#### Enabling root routes
+
+You can remove `pages` from the URL path by changing the `HighVoltage.root_routes` Boolean. For example, in `config/initializers/high_voltage.rb`:

This isn't a Boolean anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
README.md
((20 lines not shown))
You can route the root url to a high voltage page like this:
root :to => 'high_voltage/pages#show', :id => 'home'
Which will render a homepage from app/views/pages/home.html.erb
+#### Enabling root routes
+
+You can remove `pages` from the URL path by changing the `HighVoltage.root_routes` Boolean. For example, in `config/initializers/high_voltage.rb`:
+
+ HighVoltage.route_drawer = HighVoltage::RootRouteDrawer

Did you consider making this a convenience method, e.g. HighVoltage.use_root_routes? I don't mind either way, and actually this is nice because it makes it obvious how it works, but worth considering I think. Up to you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
spec/routing/routes_spec.rb
((40 lines not shown))
end
end
- context "using a custom content_path" do
+ context 'using root routing configuration' do
+ before(:all) do
+ HighVoltage.root_routes = true

Is this still the correct API for this? Or should this be HighVoltage.root_drawer = HighVoltage::RootRouteDrawer?

@harlow Collaborator
harlow added a note

Nope, good catch. Someone forgot to run the tests :flushed:

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

I'd add documentation for how to make their own route drawer, including what the API is (looks like it just returns a hash?).

@harlow
Collaborator

@gabebw that's a good idea. I wonder if its worth moving them into a route_drawer folder/module to keep them separated from the rest of the code.

lib/high_voltage/root_route_drawer.rb
@@ -0,0 +1,13 @@
+module HighVoltage
+ class RootRouteDrawer
@gabebw Owner
gabebw added a note

This could use a class-level comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/high_voltage/basic_route_drawer.rb
@@ -0,0 +1,12 @@
+module HighVoltage
+ class BasicRouteDrawer
@gabebw Owner
gabebw added a note

This could use a class-level comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/high_voltage/constraints/root_route.rb
@@ -0,0 +1,19 @@
+module HighVoltage
+ module Constraints
+ class RootRoute
@gabebw Owner
gabebw added a note

This could use a class-level comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/high_voltage/constraints/root_route.rb
((4 lines not shown))
+ def self.matches?(request)
+ File.exist? file_path(request.path)
+ end
+
+ private
+
+ def self.file_path(page_id)
+ content_path.concat("#{page_id}.html.erb")
+ end
+
+ def self.content_path
+ Rails.root.join('app', 'views', HighVoltage.content_path).to_s
+ end
+ end
+ end
+end
@gabebw Owner
gabebw added a note

ST2?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/high_voltage/basic_route_drawer.rb
@@ -0,0 +1,12 @@
+module HighVoltage
+ class BasicRouteDrawer
+ def self.match_attributes
+ {
+ "/#{HighVoltage.content_path}*id" => 'high_voltage/pages#show',
+ :as => :page,
+ :format => false
+ }
+ end
+ end
+end
+
@gabebw Owner
gabebw added a note

Please remove this blank line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
spec/routing/routes_spec.rb
((40 lines not shown))
end
end
- context "using a custom content_path" do
+ context 'using root routing configuration' do
+ before(:all) do
+ HighVoltage.root_routes = true
+ Rails.application.reload_routes!
+ end
+
+ after(:all) do
+ HighVoltage.root_routes = false
@gabebw Owner
gabebw added a note

I think this should be set to whatever it was before: HighVoltage.root_routes = @old_root_routes_setting, and set the ivar in the before(:all).

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

I don't understand how root routing differs from the regular behavior.

@harlow
Collaborator

@gabebw it differs due to the constraint -- this makes it so it won't route to the HighVoltageController if the view file does't exist. That lets us do something like this in our application:

get '/:slug', :to => 'posts#show', :as => 'post'

Anything at root that isn't caught by HighVoltage routes goes to the PostsController and hits the DB for content.

@gabebw
Owner

Ah, I see - so it's "/:id" instead of "/pages/:id" ... but only if pages/:id.html.erb exists. That could definitely use better documentation in the README.

Also - I'm pretty sure the constraint breaks if people are using Haml (or anything that's not Erb). Can you grab the Rails configuration setting and search for e.g. :id.html.haml or :id.haml or whatever's appropriate?

README.md
((29 lines not shown))
+the root of the domain path:
+
+ http://www.example.com/about
+ http://www.example.com/company
+
+Would look for corresponding files:
+
+ app/views/pages/about.html.erb
+ app/views/pages/company.html.erb
+
+This is accomplised by changing the `HighVoltage.route_drawer`. For example, in
+`config/initializers/high_voltage.rb`:
+
+ HighVoltage.route_drawer = HighVoltage::RouteDrawers::Root
+
+Note: This is not a `catch-all` route. It will check the `HighVoltage.content_path`
@gabebw Owner
gabebw added a note

catch-all isn't really code - what about just having it be "catchall" (no quotes) instead of catch-all?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/high_voltage/constraints/root_route.rb
@@ -0,0 +1,24 @@
+# Validates the request.path file exists in the views directory
@gabebw Owner
gabebw added a note

This class-level comment should be on e.g. RootRoute so that automatic documentation generators know which class it applies to. Right now it looks like documentation on the HighVoltage module.

This applies to the other class-level comments too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@gabebw gabebw commented on the diff
spec/routing/routes_spec.rb
((56 lines not shown))
+ it 'should generate normal resource route with id' do
+ page_path(:id => 'one').should eq '/one'
+ end
+
+ it 'should generate normal resource route with string' do
@gabebw Owner
gabebw added a note

Isn't this test just testing that Rails path helpers work? I don't think this is necessary with the :id => one test above.

@gabebw Owner
gabebw added a note

This applies to other tests as well.

@gabebw Owner
gabebw added a note

Actually never mind - that's not in the scope of this PR since that's from code that was already there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
spec/routing/routes_spec.rb
((52 lines not shown))
end
end
end
+
@gabebw Owner
gabebw added a note

Alas, yet another trailing newline.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/high_voltage/route_drawers/root.rb
@@ -0,0 +1,16 @@
+module HighVoltage
+ module RouteDrawers
+ # Matches routes from root of the domain e.g. http://www.example.com/about_us
+ # Uses HighVoltage::Constraits::RootRoute to validate view exists.
@gabebw Owner
gabebw added a note

Constraints is misspelled here

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

Looks good except for the typo I just noted

harlow added some commits
@harlow harlow Root routes
+ Updated readme for root routes
+ New RootRoute constraint
+ Unit test for constraints
+ Match multiple file extensions
fd43205
@harlow harlow Replace match with get in routes eea2907
@harlow harlow closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 29, 2012
  1. @harlow

    Root routes

    harlow authored
    + Updated readme for root routes
    + New RootRoute constraint
    + Unit test for constraints
    + Match multiple file extensions
  2. @harlow
This page is out of date. Refresh to see the latest.
View
37 README.md
@@ -57,20 +57,47 @@ In that case, you'd need an app/views/pages/home.html.erb file.
Generally speaking, you need to route to the 'show' action with an :id param of the view filename.
-High Voltage will generate a named route method of `page_path` which you can use, as well. If you
-want to generate a named route (with the :as routing option) for some route which will be handled
-by High Voltage, make sure not to use :page as the name, because that will conflict with the named
-route generated by High Voltage itself. For example, this will work for top-level routes (we will
+High Voltage will generate a named route method of `page_path` which you can use, as well. If you
+want to generate a named route (with the :as routing option) for some route which will be handled
+by High Voltage, make sure not to use :page as the name, because that will conflict with the named
+route generated by High Voltage itself.
+
+For example, this will work for top-level routes (we will
get a named route called `static_path` which does not conflict with the generated `page_path` method):
match '/:id' => 'high_voltage/pages#show', :as => :static, :via => :get
+#### Specifying a root path
+
You can route the root url to a high voltage page like this:
root :to => 'high_voltage/pages#show', :id => 'home'
Which will render a homepage from app/views/pages/home.html.erb
+#### Enabling root domain routes
+
+You can remove the directory `pages` from the URL path and serve up routes from
+the root of the domain path:
+
+ http://www.example.com/about
+ http://www.example.com/company
+
+Would look for corresponding files:
+
+ app/views/pages/about.html.erb
+ app/views/pages/company.html.erb
+
+This is accomplished by changing the `HighVoltage.route_drawer` to `HighVoltage::RouteDrawers::Root`
+
+ # config/initializers/high_voltage.rb
+ HighVoltage.route_drawer = HighVoltage::RouteDrawers::Root
+
+Note: This is not a catchall route. It will check the `HighVoltage.content_path`
+(by default this is `app/views/pages`) and validate the view exists.
+
+#### Disabling routes
+
The default routes can be completely removed by changing the
`HighVoltage.routes` Boolean. For example, in
`config/initializers/high_voltage.rb`:
@@ -82,7 +109,7 @@ Customize
High Voltage uses a default path and folder of 'pages', i.e. 'url.com/pages/contact' , 'app/views/pages'
-You can change this in an initializer:
+You can change this in an initializer:
HighVoltage.content_path = "site/"
View
2  config/routes.rb
@@ -1,5 +1,5 @@
Rails.application.routes.draw do
if HighVoltage.routes
- get "/#{HighVoltage.content_path}*id" => 'high_voltage/pages#show', :as => :page, :format => false
+ get HighVoltage.route_drawer.match_attributes
end
end
View
11 lib/high_voltage.rb
@@ -1,11 +1,18 @@
require 'high_voltage/version'
+require 'high_voltage/constraints/root_route'
+require 'high_voltage/page_finder'
+require 'high_voltage/route_drawers/default'
+require 'high_voltage/route_drawers/root'
module HighVoltage
mattr_accessor :layout
- @@layout = "application"
+ @@layout = 'application'
mattr_accessor :content_path
- @@content_path = "pages/"
+ @@content_path = 'pages/'
+
+ mattr_accessor :route_drawer
+ @@route_drawer = HighVoltage::RouteDrawers::Default
mattr_accessor :routes
@@routes = true
View
24 lib/high_voltage/constraints/root_route.rb
@@ -0,0 +1,24 @@
+module HighVoltage
+ module Constraints
+ # Routing constraint to validate request.path has a corresponding view
+ class RootRoute
+ VIEW_EXTENSIONS = 'html.erb,html.haml,html'
+
+ def self.matches?(request)
+ pattern = file_pattern(request.path)
+
+ Dir.glob(pattern).any?
+ end
+
+ private
+
+ def self.file_pattern(page_id)
+ "#{content_path}#{page_id}.{#{VIEW_EXTENSIONS}}"
+ end
+
+ def self.content_path
+ Rails.root.join('app', 'views', HighVoltage.content_path).to_s
+ end
+ end
+ end
+end
View
2  lib/high_voltage/engine.rb
@@ -1,6 +1,4 @@
module HighVoltage
-
class Engine < Rails::Engine
end
-
end
View
15 lib/high_voltage/route_drawers/default.rb
@@ -0,0 +1,15 @@
+module HighVoltage
+ module RouteDrawers
+ # Matches routes in the HighVoltage.content_path directory. By default this looks
+ # for /pages/id. e.g. http://www.example.com/pages/about_us
+ class Default
+ def self.match_attributes
+ {
+ "/#{HighVoltage.content_path}*id" => 'high_voltage/pages#show',
+ :as => :page,
+ :format => false
+ }
+ end
+ end
+ end
+end
View
16 lib/high_voltage/route_drawers/root.rb
@@ -0,0 +1,16 @@
+module HighVoltage
+ module RouteDrawers
+ # Matches routes from root of the domain e.g. http://www.example.com/about_us
+ # Uses HighVoltage::Constraints::RootRoute to validate view exists.
+ class Root
+ def self.match_attributes
+ {
+ "/*id" => 'high_voltage/pages#show',
+ :as => :page,
+ :format => false,
+ :constraints => HighVoltage::Constraints::RootRoute
+ }
+ end
+ end
+ end
+end
View
21 spec/constraints/root_route_spec.rb
@@ -0,0 +1,21 @@
+require 'spec_helper'
+
+describe HighVoltage::Constraints::RootRoute, '.matches?' do
+ context 'view file exists' do
+ it 'should return true' do
+ request = stub(:path => 'index')
+ Dir.stub!(:glob).and_return(['about.html.erb'])
+
+ HighVoltage::Constraints::RootRoute.matches?(request).should be_true
+ end
+ end
+
+ context 'view file does not exist' do
+ it 'should return false' do
+ request = stub(:path => 'index')
+ File.stub!(:glob).and_return([])
+
+ HighVoltage::Constraints::RootRoute.matches?(request).should be_false
+ end
+ end
+end
View
86 spec/routing/routes_spec.rb
@@ -1,37 +1,61 @@
require 'spec_helper'
-describe 'routes' do
- context "using default configuration" do
- it "should generate normal resource route with id" do
- page_path(:id => "one").should == "/pages/one"
+describe 'routes' do
+ context 'using default configuration' do
+ it 'should generate normal resource route with id' do
+ page_path(:id => 'one').should eq '/pages/one'
end
- it "should generate normal resource route with string" do
- page_path("one").should == "/pages/one"
+ it 'should generate normal resource route with string' do
+ page_path('one').should eq '/pages/one'
end
- it "should generate nested route with string" do
- page_path("one/two").should == "/pages/one/two"
+ it 'should generate nested route with string' do
+ page_path('one/two').should eq '/pages/one/two'
end
- it "should recognize nested route" do
- assert_recognizes({ :controller => "high_voltage/pages", :action => "show", :id => "one/two" }, "/pages/one/two")
+ it 'should recognize nested route' do
+ assert_recognizes({:controller => 'high_voltage/pages', :action => 'show', :id => 'one/two'}, '/pages/one/two')
end
- it "should recognize normal route" do
- assert_recognizes({ :controller => "high_voltage/pages", :action => "show", :id => "one" }, "/pages/one")
+ it 'should recognize normal route' do
+ assert_recognizes({:controller => 'high_voltage/pages', :action => 'show', :id => 'one'}, '/pages/one')
end
- it "should recognize normal route with dots" do
- assert_recognizes({ :controller => "high_voltage/pages", :action => "show", :id => "one.two.three" }, "/pages/one.two.three")
+ it 'should recognize normal route with dots' do
+ assert_recognizes({:controller => 'high_voltage/pages', :action => 'show', :id => 'one.two.three'}, '/pages/one.two.three')
end
end
- context "using a custom content_path" do
+ context 'using root routing configuration' do
+ before(:all) do
+ @original_route_drawer = HighVoltage.route_drawer
+ HighVoltage.route_drawer = HighVoltage::RouteDrawers::Root
+ Rails.application.reload_routes!
+ end
+
+ after(:all) do
+ HighVoltage.route_drawer = @original_route_drawer
+ Rails.application.reload_routes!
+ end
+
+ it 'should generate normal resource route with id' do
+ page_path(:id => 'one').should eq '/one'
+ end
+
+ it 'should generate normal resource route with string' do
@gabebw Owner
gabebw added a note

Isn't this test just testing that Rails path helpers work? I don't think this is necessary with the :id => one test above.

@gabebw Owner
gabebw added a note

This applies to other tests as well.

@gabebw Owner
gabebw added a note

Actually never mind - that's not in the scope of this PR since that's from code that was already there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ page_path('one').should eq '/one'
+ end
+
+ it 'should generate nested route with string' do
+ page_path('one/two').should eq '/one/two'
+ end
+ end
+ context 'using a custom content_path' do
before(:all) do
@original_content_path = HighVoltage.content_path
- HighVoltage.content_path = "other_pages/"
+ HighVoltage.content_path = 'other_pages/'
Rails.application.reload_routes!
end
@@ -40,32 +64,32 @@
Rails.application.reload_routes!
end
- it "should generate normal resource route with id" do
- page_path(:id => "one").should == "/other_pages/one"
+ it 'should generate normal resource route with id' do
+ page_path(:id => 'one').should eq '/other_pages/one'
end
- it "should generate normal resource route with string" do
- page_path("one").should == "/other_pages/one"
+ it 'should generate normal resource route with string' do
+ page_path('one').should eq '/other_pages/one'
end
- it "should generate nested route with string" do
- page_path("one/two").should == "/other_pages/one/two"
+ it 'should generate nested route with string' do
+ page_path('one/two').should eq '/other_pages/one/two'
end
- it "should recognize nested route" do
- assert_recognizes({:controller => "high_voltage/pages", :action => "show", :id => "one/two"}, "/other_pages/one/two")
+ it 'should recognize nested route' do
+ assert_recognizes({:controller => 'high_voltage/pages', :action => 'show', :id => 'one/two'}, '/other_pages/one/two')
end
- it "should recognize normal route" do
- assert_recognizes({:controller => "high_voltage/pages", :action => "show", :id => "one"}, "/other_pages/one")
+ it 'should recognize normal route' do
+ assert_recognizes({:controller => 'high_voltage/pages', :action => 'show', :id => 'one'}, '/other_pages/one')
end
- it "should recognize normal route with dots" do
- assert_recognizes({:controller => "high_voltage/pages", :action => "show", :id => "one.two.three"}, "/other_pages/one.two.three")
+ it 'should recognize normal route with dots' do
+ assert_recognizes({:controller => 'high_voltage/pages', :action => 'show', :id => 'one.two.three'}, '/other_pages/one.two.three')
end
end
- context "with default configuration disabled" do
+ context 'with default configuration disabled' do
around do |example|
cached_high_voltage_routes = HighVoltage.routes
HighVoltage.routes = false
@@ -75,8 +99,8 @@
Rails.application.reload_routes!
end
- it "should not recognize routes" do
- { :get => "/pages/one/two" }.should_not be_routable
+ it 'should not recognize routes' do
+ { :get => '/pages/one/two' }.should_not be_routable
end
end
end
Something went wrong with that request. Please try again.