Merge pull request #53689 from Shopify/keyword_routing_redo

gmcgibbon · web-flow · commit a6d199e283eb · 2024-11-26T00:02:32.000-06:00
Use keywords in routing mapper
diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb
diff --git a/actionpack/test/controller/resources_test.rb b/actionpack/test/controller/resources_test.rb
@@ -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
@@ -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|
diff --git a/actionpack/test/dispatch/mapper_test.rb b/actionpack/test/dispatch/mapper_test.rb
@@ -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
 
@@ -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
diff --git a/actionpack/test/dispatch/routing/concerns_test.rb b/actionpack/test/dispatch/routing/concerns_test.rb
@@ -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
diff --git a/actionpack/test/dispatch/routing_test.rb b/actionpack/test/dispatch/routing_test.rb
@@ -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
 
@@ -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
diff --git a/actionpack/test/journey/router_test.rb b/actionpack/test/journey/router_test.rb
@@ -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
@@ -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
 
