Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Oct 1, 2012

Conversation

iHiD
Copy link
Contributor

@iHiD iHiD commented May 22, 2012

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
Copy link
Contributor Author

iHiD commented May 22, 2012

@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
Copy link
Member

cc/ @josevalim @drogus

route_config << "#{" " * (regular_class_path_length - index)}end\n"
end

route route_config[2..-1]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@iHiD
Copy link
Contributor Author

iHiD commented May 27, 2012

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

@iHiD
Copy link
Contributor Author

iHiD commented Aug 23, 2012

Any update on this, @rafaelfranca? Thanks!

@schneems
Copy link
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
Copy link
Contributor Author

iHiD commented Sep 24, 2012

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
Copy link
Contributor Author

iHiD commented Sep 29, 2012

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

@schneems
Copy link
Member

👍 looks good to me.

rafaelfranca added a commit that referenced this pull request Oct 1, 2012
Master branch: Fixed generated whitespace in routes when using namespaced resource.
@rafaelfranca rafaelfranca merged commit b10c882 into rails:master Oct 1, 2012
@rafaelfranca
Copy link
Member

Merged thanks

@iHiD
Copy link
Contributor Author

iHiD commented Oct 1, 2012

Thanks @rafaelfranca - I appreciate it.

@iHiD
Copy link
Contributor Author

iHiD commented Oct 1, 2012

Do you need a 3-2 backport?

@rafaelfranca
Copy link
Member

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

@rafaelfranca
Copy link
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
Copy link
Member

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

@iHiD
Copy link
Contributor Author

iHiD commented Oct 1, 2012

You probably need to pull #7811 too. Sorry!

rafaelfranca added a commit that referenced this pull request Oct 1, 2012
Master branch: Fixed generated whitespace in routes when using namespaced resource.

Merge pull request #7811 from iHiD/resource_generator_routes_master

Fix the build (Broken scaffold routes test)
@rafaelfranca
Copy link
Member

Done at a02f67b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants