Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Master branch: Fixed generated whitespace in routes when using namespaced resource. #6450

Merged
merged 1 commit into from

4 participants

@iHiD

A version of #6449 on the master branch.

Currently, running rails generate resource admin/blog_post gives the following:

MyApplication::Application.routes.draw do
  namespace :admin do resources :blog_posts end
end

This patch fixes the whitespace, so it's generated as follows:

MyApplication::Application.routes.draw do
  namespace :admin do
    resources :blog_posts
  end
end

It's something that's been bugging me for ages, so I figured it was worth fixing.

All feedback welcome. Thanks guys!

@iHiD

@rafaelfranca I've updated the code as per your comments on #6449. Any feedback? It's not a particularly pretty implementation but I think it's efficient. Thanks.

...tors/rails/resource_route/resource_route_generator.rb
((7 lines not shown))
- route route_config
+
+ regular_class_path_length = regular_class_path.length
+
+ route_config = ""
+ regular_class_path.each_with_index do |namespace, index|
+ route_config << "#{" " * (index + 1)}namespace :#{namespace} do\n"
+ end
+
+ route_config << " #{" " * regular_class_path_length}resources :#{file_name.pluralize}\n"
+
+ regular_class_path.each_index do |index|
+ route_config << "#{" " * (regular_class_path_length - index)}end\n"
+ end
+
+ route route_config[2..-1]
@iHiD
iHiD added a note

Was tempted to put a comment on this line. route prepends two spaces onto the front of the string that is passed, so we need to strip those spaces off. We could edit the initial loop to do this, but I couldn't think of a non-messy way to do it. Maybe route shouldn't prepend the spaces?

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

Any further thoughts on this? Anything you'd like me to change? Thanks.

@iHiD

Any update on this, @rafaelfranca? Thanks!

@schneems
Collaborator

Thanks for working on this, I poked around in the code and would like to see a slightly more abstracted solution. I took the liberty of playing with your solution and something like this would be more maintainable into the future:

module Rails
  module Generators
    class ResourceRouteGenerator < NamedBase

      def route_string
        @route_string ||= ""
      end

      def write(str = '', indent = 0)
        route_string << "#{"  " * indent}#{str}\n"
      end

      def route_length
        regular_class_path.length
      end


      # Properly nests namespaces passed into a generator
      #
      #   $ rails generate resource admin/users/products
      #
      # should give you
      #
      #   namespace :admin do
      #     namespace :users
      #       resources :products
      #     end
      #   end
      def add_resource_route
        return if options[:actions].present?

        # iterates over all namespaces and opens up blocks
        regular_class_path.each_with_index do |namespace, index|
          write("namespace :#{namespace} do", index + 1)
        end

        # inserts the primary resource
        write("resources :#{file_name.pluralize}", route_length + 2)

        # ends blocks
        regular_class_path.each_index do |index|
          write("end", route_length - index)
        end

        # route prepends two spaces onto the front of the string that is passed, this corrects that
        route route_string[2..-1]
      end
    end
  end
end

You'll want to verify the whitespace calculations on this still work out and maybe modify your tests. When you're done you'll need to squash to one commit. I can help you out with any of that if you have questions/problems. If you're not interested in working on this anymore let me know and i'll put this in a new PR. Either way, thanks and let me know if you have questions.

@iHiD

Great work. Thanks, @schneems. I'm just off to bed, but I'll take a look tomorrow morning and put a squashed commit together, with some updated tests. Cheers!

@iHiD

I've updated this as per @schneems suggestions. Anything stopping this from getting pulled now? Thanks! /cc @rafaelfranca, @drogus

@schneems
Collaborator

:+1: looks good to me.

@rafaelfranca rafaelfranca merged commit b10c882 into rails:master
@rafaelfranca
Owner

Merged thanks

@iHiD

Thanks @rafaelfranca - I appreciate it.

@iHiD

Do you need a 3-2 backport?

@rafaelfranca
Owner

This pull request broke the build. Please fix it. https://travis-ci.org/#!/rails/rails/jobs/2615327

@rafaelfranca
Owner

After the railties tests green at the master branch I will backport. I can fix the tests but I don't have time right now.

@steveklabnik
Collaborator

@rafaelfranca should be fixed now, feel free to backport.

@iHiD

You probably need to pull #7811 too. Sorry!

@rafaelfranca
Owner

Done at a02f67b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
45 railties/lib/rails/generators/rails/resource_route/resource_route_generator.rb
@@ -1,13 +1,50 @@
module Rails
module Generators
class ResourceRouteGenerator < NamedBase
+
+ # Properly nests namespaces passed into a generator
+ #
+ # $ rails generate resource admin/users/products
+ #
+ # should give you
+ #
+ # namespace :admin do
+ # namespace :users
+ # resources :products
+ # end
+ # end
def add_resource_route
return if options[:actions].present?
- route_config = regular_class_path.collect{ |namespace| "namespace :#{namespace} do " }.join(" ")
- route_config << "resources :#{file_name.pluralize}"
- route_config << " end" * regular_class_path.size
- route route_config
+
+ # iterates over all namespaces and opens up blocks
+ regular_class_path.each_with_index do |namespace, index|
+ write("namespace :#{namespace} do", index + 1)
+ end
+
+ # inserts the primary resource
+ write("resources :#{file_name.pluralize}", route_length + 1)
+
+ # ends blocks
+ regular_class_path.each_index do |index|
+ write("end", route_length - index)
+ end
+
+ # route prepends two spaces onto the front of the string that is passed, this corrects that
+ route route_string[2..-1]
end
+
+ private
+ def route_string
+ @route_string ||= ""
+ end
+
+ def write(str, indent)
+ route_string << "#{" " * indent}#{str}\n"
+ end
+
+ def route_length
+ regular_class_path.length
+ end
end
end
end
View
76 railties/test/generators/namespaced_generators_test.rb
@@ -304,7 +304,7 @@ def test_scaffold_with_namespace_on_invoke
# Route
assert_file "config/routes.rb" do |route|
- assert_match(/namespace :admin do resources :roles end$/, route)
+ assert_match(/^ namespace :admin do\n resources :roles\n end$/, route)
end
# Controller
@@ -346,7 +346,7 @@ def test_scaffold_with_namespace_on_revoke
# Route
assert_file "config/routes.rb" do |route|
- assert_no_match(/namespace :admin do resources :roles end$/, route)
+ assert_no_match(/^ namespace :admin do\n resources :roles\n end$$/, route)
end
# Controller
@@ -364,4 +364,76 @@ def test_scaffold_with_namespace_on_revoke
# Stylesheets (should not be removed)
assert_file "app/assets/stylesheets/scaffold.css"
end
+
+ def test_scaffold_with_nested_namespace_on_invoke
+ run_generator [ "admin/user/special/role", "name:string", "description:string" ]
+
+ # Model
+ assert_file "app/models/test_app/admin/user/special.rb", /module TestApp\n module Admin/
+ assert_file "app/models/test_app/admin/user/special/role.rb", /module TestApp\n class Admin::User::Special::Role < ActiveRecord::Base/
+ assert_file "test/unit/test_app/admin/user/special/role_test.rb", /module TestApp\n class Admin::User::Special::RoleTest < ActiveSupport::TestCase/
+ assert_file "test/fixtures/test_app/admin/user/special/roles.yml"
+ assert_migration "db/migrate/create_test_app_admin_user_special_roles.rb"
+
+ # Route
+ assert_file "config/routes.rb" do |route|
+ assert_match(/^ namespace :admin do\n namespace :user do\n namespace :special do\n resources :roles\n end\n end\n end$/, route)
+ end
+
+ # Controller
+ assert_file "app/controllers/test_app/admin/user/special/roles_controller.rb" do |content|
+ assert_match(/module TestApp\n class Admin::User::Special::RolesController < ApplicationController/, content)
+ end
+
+ assert_file "test/functional/test_app/admin/user/special/roles_controller_test.rb",
+ /module TestApp\n class Admin::User::Special::RolesControllerTest < ActionController::TestCase/
+
+ # Views
+ %w(
+ index
+ edit
+ new
+ show
+ _form
+ ).each { |view| assert_file "app/views/test_app/admin/user/special/roles/#{view}.html.erb" }
+ assert_no_file "app/views/layouts/admin/user/special/roles.html.erb"
+
+ # Helpers
+ assert_file "app/helpers/test_app/admin/user/special/roles_helper.rb"
+ assert_file "test/unit/helpers/test_app/admin/user/special/roles_helper_test.rb"
+
+ # Stylesheets
+ assert_file "app/assets/stylesheets/scaffold.css"
+ end
+
+ def test_scaffold_with_nested_namespace_on_revoke
+ run_generator [ "admin/user/special/role", "name:string", "description:string" ]
+ run_generator [ "admin/user/special/role" ], :behavior => :revoke
+
+ # Model
+ assert_file "app/models/test_app/admin/user/special.rb" # ( should not be remove )
+ assert_no_file "app/models/test_app/admin/user/special/role.rb"
+ assert_no_file "test/unit/test_app/admin/user/special/role_test.rb"
+ assert_no_file "test/fixtures/test_app/admin/user/special/roles.yml"
+ assert_no_migration "db/migrate/create_test_app_admin_user_special_roles.rb"
+
+ # Route
+ assert_file "config/routes.rb" do |route|
+ assert_no_match(/^ namespace :admin do\n namespace :user do\n namespace :special do\n resources :roles\n end\n end\n end$/, route)
+ end
+
+ # Controller
+ assert_no_file "app/controllers/test_app/admin/user/special/roles_controller.rb"
+ assert_no_file "test/functional/test_app/admin/user/special/roles_controller_test.rb"
+
+ # Views
+ assert_no_file "app/views/test_app/admin/user/special/roles"
+
+ # Helpers
+ assert_no_file "app/helpers/test_app/admin/user/special/roles_helper.rb"
+ assert_no_file "test/unit/helpers/test_app/admin/user/special/roles_helper_test.rb"
+
+ # Stylesheets (should not be removed)
+ assert_file "app/assets/stylesheets/scaffold.css"
+ end
end
Something went wrong with that request. Please try again.