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

Merged
merged 1 commit into from Oct 1, 2012

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.

@rafaelfranca
Ruby on Rails member
@iHiD iHiD commented on an outdated diff May 22, 2012
...tors/rails/resource_route/resource_route_generator.rb
- 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 May 22, 2012

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
Ruby on Rails member

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
Ruby on Rails member

👍 looks good to me.

@rafaelfranca rafaelfranca merged commit b10c882 into rails:master Oct 1, 2012
@rafaelfranca
Ruby on Rails member

Merged thanks

@iHiD

Thanks @rafaelfranca - I appreciate it.

@iHiD

Do you need a 3-2 backport?

@rafaelfranca
Ruby on Rails member

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

@rafaelfranca
Ruby on Rails member

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
Ruby on Rails member

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

@iHiD

You probably need to pull #7811 too. Sorry!

@rafaelfranca
Ruby on Rails member

Done at a02f67b

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