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

Already on GitHub? Sign in to your account

Add ability to set a prefix name for routes which have hyphen #12598

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants
Contributor

amrnt commented Oct 21, 2013

With current Rails when you have in your routes

get '/contact_us' => 'pages#contact'
get '/about_us'   => 'pages#about_us'

you'll get this inspection:

           Prefix Verb  URI Pattern                  Controller#Action
       contact_us GET   /contact_us(.:format)        pages#contact
         about_us GET   /about_us(.:format)          pages#about_us

But when your routes are with hyphen (dash), like so:

get '/contact-us' => 'pages#contact'
get '/about-us'   => 'pages#about_us'

you'll get this:

           Prefix Verb  URI Pattern                  Controller#Action
                  GET   /contact-us(.:format)        pages#contact
                  GET   /about-us(.:format)          pages#about_us

Which does lead that theres no helper methods for these routes i.e. about_us_path, contact_us_path.

For this pull request, it will convert the hyphens to underscores in paths so it'll be familiar to Ruby to translate it to a method.

This is will be the inspection for the latest routes above:

           Prefix Verb  URI Pattern                  Controller#Action
       contact_us GET   /contact-us(.:format)        pages#contact
         about_us GET   /about-us(.:format)          pages#about_us
Member

senny commented Oct 21, 2013

Owner

pixeltrix commented Oct 21, 2013

@amrnt I'm 👍 on the idea but I think the implementation can be simpler - keep your tests as is but undo your code changes and then change Mapper.normalize_name to this:

def self.normalize_name(name)
  normalize_path(name)[1..-1].tr("/-", "_")
end

this should make the tests pass - if not let me know and I'll take another look. Can you also add a test for shorthand routes as well - i.e. ones without a :to option.

Contributor

amrnt commented Oct 21, 2013

All set 😃

@pixeltrix pixeltrix commented on the diff Oct 21, 2013

actionpack/lib/action_dispatch/routing/mapper.rb
@@ -1429,7 +1429,7 @@ def add_route(action, options) # :nodoc:
path = path_for_action(action, options.delete(:path))
action = action.to_s.dup
- if action =~ /^[\w\/]+$/
+ if action =~ /^[\w\-\/]+$/
@pixeltrix

pixeltrix Oct 21, 2013

Owner

Does this need to be there?

@amrnt

amrnt Oct 21, 2013

Contributor

yes it's need, else it will not treat a dashed-word as a word. See http://rubular.com/r/O59hUZTshN and try to remove the dash -\

@pixeltrix pixeltrix and 1 other commented on an outdated diff Oct 21, 2013

actionpack/test/dispatch/routing/inspector_test.rb
@@ -57,11 +57,15 @@ def self.inspect
def test_cart_inspect
output = draw do
get '/cart', :to => 'cart#show'
+ get '/view-cart', :to => 'cart#show'
+ get '/view-pending-cart' => 'cart#show_pending'
@pixeltrix

pixeltrix Oct 21, 2013

Owner

Can you add a route without a endpoint, e.g.:

get '/about-us'

That should go to AboutUsController#index and have the url helper name about_us_url.

@amrnt

amrnt Oct 21, 2013

Contributor

well, without an endpoint it will throw ArgumentError: missing :controller

@amrnt

amrnt Oct 21, 2013

Contributor

it'll throw same error for

get '/about_us'
Contributor

josephzidell commented Oct 22, 2013

What is you have get '/contact_us' => '...' and get '/contact-us' => '...'

Contributor

amrnt commented Oct 22, 2013

@websiteswithclass both will have same path helper method (but you know, one will override another).

This doesn't merge cleanly anymore. Also can you please add a changelog entry?

@pixeltrix wanna take another look?

I'm wondering about this vs adding as: about_us as we do today. Thanks.

Contributor

amrnt commented Dec 20, 2013

@carlosantoniodasilva I'm not sure if what I did now is fine. I merged from upstream/master into this feature branch and fix the merge conflict. After, I fix where the test fails.

Also, I added the changes to the changelog.

About using as: :about_us, it gives the same you know, but since a prefix name generated automatic for underscored routes why not to for hyphened routes.

@ghost ghost assigned pixeltrix Jan 5, 2014

Contributor

mellowi commented Jan 20, 2014

Tested that the fix works as expected (gives correct helper method) and the tests are looking good. The fix seems legit.

Owner

pixeltrix commented Jan 20, 2014

Hmm, I wonder what happens when you use shorthand routes, e.g. get '/about-us' - with the controller and action inferred from the path. We should have tests to ensure what happens is what we want.

mellowi added a commit to mellowi/rails that referenced this pull request Jan 20, 2014

Merge pull request #12598 from amrnt/rails
Adding hyphen support for route (dashes transformed to underscores)
Contributor

mellowi commented Jan 20, 2014

Fixed the issue that @pixeltrix was worried about. Still the route needs both, the controller and action defined to work eg. get '/about-us/index'

Contributor

amrnt commented Jan 20, 2014

@pixeltrix, The addition that @mellowi made just works fine. get '/about-us/index' will map to about_us#index.

I can add it to my pull request (with tests), but how to give @mellowi a credit on this?

Owner

pixeltrix commented Jan 20, 2014

@amrnt @mellowi here's what I'll do - I will cherry pick each of your commits and push it manually. So each of you finish your respective bits and I will clean it up and give everyone credit.

mellowi added a commit to mellowi/rails that referenced this pull request Jan 20, 2014

Fixes #12598
Adding hyphen support for route (dashes transformed to underscores)

pixeltrix added a commit that referenced this pull request Jan 20, 2014

Owner

pixeltrix commented Jan 20, 2014

Closed by the following commits: 746abbc, f9f32e0 and bf19131

@pixeltrix pixeltrix closed this Jan 20, 2014

Contributor

amrnt commented Jan 20, 2014

👍 Awesome @pixeltrix! ❤️ 💙 💚 💛 💜

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