Skip to content

Commit

Permalink
Use keywords in routing mapper
Browse files Browse the repository at this point in the history
Converts hashes to keywords in ActionDispatch::Routing::Mapper. This
makes the mapper a little easier to reason about, and slightly faster.
  • Loading branch information
gmcgibbon committed Nov 25, 2024
1 parent 9add4d9 commit 0583415
Show file tree
Hide file tree
Showing 6 changed files with 393 additions and 202 deletions.
488 changes: 323 additions & 165 deletions actionpack/lib/action_dispatch/routing/mapper.rb

Large diffs are not rendered by default.

8 changes: 3 additions & 5 deletions actionpack/test/controller/resources_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ def test_multiple_default_restful_routes
def test_multiple_resources_with_options
expected_options = { controller: "threads", action: "index" }

with_restful_routing :messages, :comments, expected_options.slice(:controller) do
with_restful_routing :messages, :comments, controller: "threads" do
assert_recognizes(expected_options, path: "comments")
assert_recognizes(expected_options, path: "messages")
end
Expand Down Expand Up @@ -1176,17 +1176,15 @@ def test_invalid_except_option_for_singleton_resource
end

private
def with_restful_routing(*args)
options = args.extract_options!
def with_restful_routing(*args, **options)
collection_methods = options.delete(:collection)
member_methods = options.delete(:member)
path_prefix = options.delete(:path_prefix)
args.push(options)

with_routing do |set|
set.draw do
scope(path_prefix || "") do
resources(*args) do
resources(*args, **options) do
if collection_methods
collection do
collection_methods.each do |name, method|
Expand Down
59 changes: 58 additions & 1 deletion actionpack/test/dispatch/mapper_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ def test_mapping_requirements
options = {}
scope = Mapper::Scope.new({})
ast = Journey::Parser.parse "/store/:name(*rest)"
m = Mapper::Mapping.build(scope, FakeSet.new, ast, "foo", "bar", nil, [:get], nil, {}, true, options)
m = Mapper::Mapping.build(scope, FakeSet.new, ast, "foo", "bar", nil, [:get], nil, {}, true, nil, options)
assert_equal(/.+?/m, m.requirements[:rest])
end

Expand Down Expand Up @@ -225,6 +225,63 @@ def test_scope_does_not_destructively_mutate_default_options
end
end
end

def test_deprecated_hash
fakeset = FakeSet.new
mapper = Mapper.new fakeset

assert_deprecated(ActionDispatch.deprecator) do
mapper.get "/foo", { to: "home#index" }
end

assert_deprecated(ActionDispatch.deprecator) do
mapper.post "/foo", { to: "home#index" }
end

assert_deprecated(ActionDispatch.deprecator) do
mapper.put "/foo", { to: "home#index" }
end

assert_deprecated(ActionDispatch.deprecator) do
mapper.patch "/foo", { to: "home#index" }
end

assert_deprecated(ActionDispatch.deprecator) do
mapper.delete "/foo", { to: "home#index" }
end

assert_deprecated(ActionDispatch.deprecator) do
mapper.options "/foo", { to: "home#index" }
end

assert_deprecated(ActionDispatch.deprecator) do
mapper.connect "/foo", { to: "home#index" }
end

assert_deprecated(ActionDispatch.deprecator) do
mapper.match "/foo", { to: "home#index", via: :get }
end

assert_deprecated(ActionDispatch.deprecator) do
mapper.mount(lambda { |env| [200, {}, [""]] }, { at: "/" })
end

assert_deprecated(ActionDispatch.deprecator) do
mapper.scope("/hello", { only: :get }) { }
end

assert_deprecated(ActionDispatch.deprecator) do
mapper.namespace(:admin, { module: "sekret" }) { }
end

assert_deprecated(ActionDispatch.deprecator) do
mapper.resource(:user, { only: :show }) { }
end

assert_deprecated(ActionDispatch.deprecator) do
mapper.resources(:users, { only: :show }) { }
end
end
end
end
end
4 changes: 2 additions & 2 deletions actionpack/test/dispatch/routing/concerns_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,14 @@ class ReviewsController < ResourcesController; end
class RoutingConcernsTest < ActionDispatch::IntegrationTest
class Reviewable
def self.call(mapper, options = {})
mapper.resources :reviews, options
mapper.resources :reviews, **options
end
end

Routes = ActionDispatch::Routing::RouteSet.new.tap do |app|
app.draw do
concern :commentable do |options|
resources :comments, options
resources :comments, **options
end

concern :image_attachable do
Expand Down
26 changes: 2 additions & 24 deletions actionpack/test/dispatch/routing_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -988,13 +988,13 @@ def test_resources_for_uncountable_names

def test_resource_does_not_modify_passed_options
options = { id: /.+?/, format: /json|xml/ }
draw { resource :user, options }
draw { resource :user, **options }
assert_equal({ id: /.+?/, format: /json|xml/ }, options)
end

def test_resources_does_not_modify_passed_options
options = { id: /.+?/, format: /json|xml/ }
draw { resources :users, options }
draw { resources :users, **options }
assert_equal({ id: /.+?/, format: /json|xml/ }, options)
end

Expand Down Expand Up @@ -2107,32 +2107,10 @@ def test_resource_merges_options_from_scope
end
end

assert_equal "/account", account_path
assert_raise(NoMethodError) { new_account_path }

get "/account/new"
assert_equal 404, status

get "/account"
assert_equal 200, status
end

def test_resource_merges_options_from_scope_hash
draw do
scope_options = { only: :show }
scope scope_options do
resource :account
end
end

assert_equal "/account", account_path
assert_raise(NoMethodError) { new_account_path }

get "/account/new"
assert_equal 404, status

get "/account"
assert_equal 200, status
end

def test_resources_merges_options_from_scope
Expand Down
10 changes: 5 additions & 5 deletions actionpack/test/journey/router_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ def test_nil_path_parts_are_ignored
def test_generate_slash
params = [ [:controller, "tasks"],
[:action, "show"] ]
get "/", Hash[params]
get "/", **Hash[params]

path, _ = _generate(nil, Hash[params], {})
assert_equal "/", path
Expand Down Expand Up @@ -498,15 +498,15 @@ def _generate(route_name, options, recall)
[uri.path, params]
end

def get(*args)
def get(...)
ActionDispatch.deprecator.silence do
mapper.get(*args)
mapper.get(...)
end
end

def match(*args)
def match(...)
ActionDispatch.deprecator.silence do
mapper.match(*args)
mapper.match(...)
end
end

Expand Down

0 comments on commit 0583415

Please sign in to comment.