Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

Unify routes naming by renaming router to routes

Signed-off-by: José Valim <jose.valim@gmail.com>
  • Loading branch information...
commit f7ba614c2db31933cbc12eda87518de3eca0228c 1 parent f8720a0
@drogus drogus authored josevalim committed
View
4 actionmailer/lib/action_mailer/base.rb
@@ -737,13 +737,13 @@ def deprecated_url_options
raise "You can no longer call ActionMailer::Base.default_url_options " \
"directly. You need to set config.action_mailer.default_url_options. " \
"If you are using ActionMailer standalone, you need to include the " \
- "url_helpers of a router directly."
+ "url_helpers of a routes directly."
@wincent
wincent added a note

"of a routes" isn't valid grammatically. What are you trying to say here? "of the routes"? Or are you really saying "of the router"?

@josevalim Owner

I think we could rename it as: "you need to include routes' url_helpers directly". What do you think? Could you please provide a patch?

@wincent
wincent added a note

That's not valid grammatically either. "routes" is not a pronoun, so you can't use it as a bare possessive like that (without "the").

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
end
end
# This module will complain if the user tries to set default_url_options
# directly instead of through the config object. In Action Mailer's Railtie,
- # we include the url_helpers of the router, which will override this module
+ # we include the url_helpers of the routes, which will override this module
extend DeprecatedUrlOptions
ActiveSupport.run_load_hooks(:action_mailer, self)
View
4 actionpack/lib/abstract_controller/rendering.rb
@@ -50,8 +50,8 @@ def view_context_class
if controller.respond_to?(:_helpers)
include controller._helpers
- if controller.respond_to?(:_router)
- include controller._router.url_helpers
+ if controller.respond_to?(:_routes)
+ include controller._routes.url_helpers
end
# TODO: Fix RJS to not require this
View
2  actionpack/lib/action_controller/deprecated/base.rb
@@ -96,7 +96,7 @@ def resource_action_separator
def resource_action_separator=(val)
ActiveSupport::Deprecation.warn "ActionController::Base.resource_action_separator is deprecated and only " \
- "works with the deprecated router DSL."
+ "works with the deprecated routes DSL."
@resource_action_separator = val
end
View
2  actionpack/lib/action_controller/metal.rb
@@ -47,7 +47,7 @@ def build(action, app=nil, &block)
#
# In AbstractController, dispatching is triggered directly by calling #process on a new controller.
# ActionController::Metal provides an #action method that returns a valid Rack application for a
- # given action. Other rack builders, such as Rack::Builder, Rack::URLMap, and the Rails router,
+ # given action. Other rack builders, such as Rack::Builder, Rack::URLMap, and the Rails routes,
# can dispatch directly to the action returned by FooController.action(:index).
class Metal < AbstractController::Base
abstract!
View
8 actionpack/lib/action_controller/metal/url_for.rb
@@ -12,17 +12,17 @@ def url_options
).merge(:script_name => request.script_name)
end
- def _router
+ def _routes
raise "In order to use #url_for, you must include the helpers of a particular " \
- "router. For instance, `include Rails.application.routes.url_helpers"
+ "routes. For instance, `include Rails.application.routes.url_helpers"
@wincent
wincent added a note

Again, "of a particular routes" isn't grammatically valid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
end
module ClassMethods
def action_methods
@action_methods ||= begin
- super - _router.named_routes.helper_names
+ super - _routes.named_routes.helper_names
end
end
end
end
-end
+end
View
4 actionpack/lib/action_dispatch/routing/deprecated_mapper.rb
@@ -30,8 +30,8 @@ def in_memory_controller_namespaces
class DeprecatedMapper #:nodoc:
def initialize(set) #:nodoc:
- ActiveSupport::Deprecation.warn "You are using the old router DSL which will be removed in Rails 3.1. " <<
- "Please check how to update your router file at: http://www.engineyard.com/blog/2010/the-lowdown-on-routes-in-rails-3/"
+ ActiveSupport::Deprecation.warn "You are using the old routes DSL which will be removed in Rails 3.1. " <<
+ "Please check how to update your routes file at: http://www.engineyard.com/blog/2010/the-lowdown-on-routes-in-rails-3/"
@set = set
end
View
2  actionpack/lib/action_dispatch/routing/mapper.rb
@@ -308,7 +308,7 @@ def scope(*args)
if name_prefix = options.delete(:name_prefix)
options[:as] ||= name_prefix
- ActiveSupport::Deprecation.warn ":name_prefix was deprecated in the new router syntax. Use :as instead.", caller
+ ActiveSupport::Deprecation.warn ":name_prefix was deprecated in the new routes syntax. Use :as instead.", caller
end
case args.first
View
6 actionpack/lib/action_dispatch/routing/route_set.rb
@@ -268,10 +268,10 @@ class << self
# Yes plz - JP
included do
routes.install_helpers(self)
- singleton_class.send(:define_method, :_router) { routes }
+ singleton_class.send(:define_method, :_routes) { routes }
end
- define_method(:_router) { routes }
+ define_method(:_routes) { routes }
end
helpers
@@ -474,7 +474,7 @@ def url_for(options)
path_options = yield(path_options) if block_given?
path = generate(path_options, path_segments || {})
- # ROUTES TODO: This can be called directly, so script_name should probably be set in the router
+ # ROUTES TODO: This can be called directly, so script_name should probably be set in routes
rewritten_url << (options[:trailing_slash] ? path.sub(/\?|\z/) { "/" + $& } : path)
rewritten_url << "##{Rack::Mount::Utils.escape_uri(options[:anchor].to_param.to_s)}" if options[:anchor]
View
2  actionpack/lib/action_dispatch/routing/url_for.rb
@@ -128,7 +128,7 @@ def url_for(options = nil)
when String
options
when nil, Hash
- _router.url_for(url_options.merge((options || {}).symbolize_keys))
+ _routes.url_for(url_options.merge((options || {}).symbolize_keys))
else
polymorphic_url(options)
end
View
2  actionpack/lib/action_view/base.rb
@@ -173,7 +173,7 @@ module Subclasses
@@field_error_proc = Proc.new{ |html_tag, instance| "<div class=\"field_with_errors\">#{html_tag}</div>".html_safe }
class_attribute :helpers
- class_attribute :_router
+ class_attribute :_routes
class << self
delegate :erb_trim_mode=, :to => 'ActionView::Template::Handlers::ERB'
View
2  actionpack/lib/action_view/helpers/url_helper.rb
@@ -12,7 +12,7 @@ module Helpers #:nodoc:
# and controllers.
module UrlHelper
# This helper may be included in any class that includes the
- # URL helpers of a router (router.url_helpers). Some methods
+ # URL helpers of a routes (routes.url_helpers). Some methods
# provided here will only work in the4 context of a request
# (link_to_unless_current, for instance), which must be provided
# as a method called #request on the context.
View
10 actionpack/lib/action_view/test_case.rb
@@ -151,7 +151,7 @@ def view
@view ||= begin
view = ActionView::Base.new(ActionController::Base.view_paths, {}, @controller)
view.singleton_class.send :include, _helpers
- view.singleton_class.send :include, @controller._router.url_helpers
+ view.singleton_class.send :include, @controller._routes.url_helpers
view.singleton_class.send :delegate, :alert, :notice, :to => "request.flash"
view.extend(Locals)
view.locals = self.locals
@@ -192,13 +192,13 @@ def _assigns
end
end
- def _router
- @controller._router if @controller.respond_to?(:_router)
+ def _routes
+ @controller._routes if @controller.respond_to?(:_routes)
end
def method_missing(selector, *args)
- if @controller.respond_to?(:_router) &&
- @controller._router.named_routes.helpers.include?(selector)
+ if @controller.respond_to?(:_routes) &&
+ @controller._routes.named_routes.helpers.include?(selector)
@controller.__send__(selector, *args)
else
super
View
8 actionpack/test/abstract_unit.rb
@@ -41,7 +41,7 @@
module Rails
end
-# Monkey patch the old router initialization to be silenced.
+# Monkey patch the old routes initialization to be silenced.
class ActionDispatch::Routing::DeprecatedMapper
def initialize_with_silencer(*args)
ActiveSupport::Deprecation.silence { initialize_without_silencer(*args) }
@@ -275,9 +275,9 @@ def assert_header(name, value)
class ActionController::Base
def self.test_routes(&block)
- router = ActionDispatch::Routing::RouteSet.new
- router.draw(&block)
- include router.url_helpers
+ routes = ActionDispatch::Routing::RouteSet.new
+ routes.draw(&block)
+ include routes.url_helpers
end
end
View
2  actionpack/test/controller/url_for_test.rb
@@ -120,7 +120,7 @@ def test_trailing_slash_with_params
def test_relative_url_root_is_respected
# ROUTES TODO: Tests should not have to pass :relative_url_root directly. This
- # should probably come from the router.
+ # should probably come from routes.
add_host!
assert_equal('https://www.basecamphq.com/subdir/c/a/i',
View
16 actionpack/test/dispatch/url_generation_test.rb
@@ -2,24 +2,24 @@
module TestUrlGeneration
class WithMountPoint < ActionDispatch::IntegrationTest
- Router = ActionDispatch::Routing::RouteSet.new
- Router.draw { match "/foo", :to => "my_route_generating#index", :as => :foo }
+ Routes = ActionDispatch::Routing::RouteSet.new
+ Routes.draw { match "/foo", :to => "my_route_generating#index", :as => :foo }
class ::MyRouteGeneratingController < ActionController::Base
- include Router.url_helpers
+ include Routes.url_helpers
def index
render :text => foo_path
end
end
- include Router.url_helpers
+ include Routes.url_helpers
- def _router
- Router
+ def _routes
+ Routes
end
def app
- Router
+ Routes
end
test "generating URLS normally" do
@@ -30,7 +30,7 @@ def app
assert_equal "/bar/foo", foo_path(:script_name => "/bar")
end
- test "the request's SCRIPT_NAME takes precedence over the router's" do
+ test "the request's SCRIPT_NAME takes precedence over the routes'" do
get "/foo", {}, 'SCRIPT_NAME' => "/new"
assert_equal "/new/foo", response.body
end

7 comments on commit f7ba614

@wincent

I've added a couple of line comments to this commit above but I keep seeing the same issues over and over again so will just comment here in general.

While the basic idea of replacing "router" with "routes" at the code level seems fine, it looks like this is an over-zealous find-and-replace that has introduced two kinds of problem:

One problem is the bunch of grammatical errors, where the change adds phrases of the form "of a routes", which aren't valid grammatically.

The other problem is that there actually is a thing in Rails called "the router" and it is referred to all over the Internet, including places like the official Rails 3 release notes (http://edgeguides.rubyonrails.org/3_0_release_notes.html), which list the number one feature of this release as "Brand new router".

So this change in many places substitutes "routes" for "router" where it shouldn't, ie. it's always been called the "router DSL", not the "routes DSL" (just look at the number of times it was referred to as "the new router" or "the new router DSL" in the recent RailsConf). Or as another example, "Other rack builders, such as Rack::Builder, Rack::URLMap, and the Rails routes"; it's not the "routes" which are the Rack builder, but the thing widely referred to as "the router".

By all means change the code-level instances to unify variable, class, and method naming where appropriate, but don't break valid English syntax and introduce an unnecessary change at the abstract level when referring to the thing we all, until now, have referred to as "the Rails router".

@josevalim
Owner

Wincent, thanks for the sanity check! I completely agree with you and this should be considered a bug as any other issue in the code. That said, you could please fix up these issues in a patch? I will apply it as soon as I can!

@wincent

Will see if I can cook something up.

@drogus
Collaborator

wincent: thanks for your comments, I'm looking forward for your solution

the problem is, router and routes were used for the same thing in code, which could confuse people. Consider such code:
env["action_dispatch.routes"] = self.routes

and then:
env["action_dispathc.routes"].equals?(_router)

_router and routes can refer to the same object, but are named differently, which does not follow any logical pattern (please correct me if I'm wrong)

Maybe we should allways use routes in code and use "router" in cases where it's better from grammatical point of view (in documentation or code comments)?

@wincent

Yes, that's exactly what I was trying to say. At the code level I think the change is fine, but in the documentation, code comments, and user-visible strings, we need to be careful. Give me a few minutes and I'll come up with a proposed revision.

@drogus
Collaborator

Thanks! :D

(I haven't noticed your note with agreement on code-level changes, sorry for duplicating your thoughts in my last comment :)

@wincent

Ok, here's a suggested fix:

http://gist.github.com/462489

Cheers,
Wincent

Please sign in to comment.
Something went wrong with that request. Please try again.