Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

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

Closed
wants to merge 1 commit into from

6 participants

@amrnt

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
@senny
Owner
@pixeltrix
Owner

@amrnt I'm :+1: 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.

@amrnt

All set :smiley:

@pixeltrix pixeltrix commented on the diff
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 Owner

Does this need to be there?

@amrnt
amrnt added a note

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 -\

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 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 added a note

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

@amrnt
amrnt added a note

it'll throw same error for

get '/about_us'
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@josephzidell

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

@amrnt

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

@carlosantoniodasilva

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.

@amrnt

@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.

@pixeltrix pixeltrix was assigned
@mellowi

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

@pixeltrix
Owner

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

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'

@amrnt

@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?

@pixeltrix
Owner

@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 mellowi referenced this pull request from a commit in mellowi/rails
@mellowi mellowi Fixes #12598
Adding hyphen support for route (dashes transformed to underscores)
b3fc686
@pixeltrix
Owner

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

@pixeltrix pixeltrix closed this
@amrnt

:+1: Awesome @pixeltrix! :heart: :blue_heart: :green_heart: :yellow_heart: :purple_heart:

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.
13 actionpack/CHANGELOG.md
@@ -1,3 +1,16 @@
+* Add ability to set a prefix name for routes which have hyphen(s).
+
+ get '/contact-us' => 'pages#contact'
+ get '/about-us' => 'pages#about_us'
+
+ The above routes will inspected to
+
+ Prefix Verb URI Pattern Controller#Action
+ contact_us GET /contact-us(.:format) pages#contact
+ about_us GET /about-us(.:format) pages#about_us
+
+ *Amr Tamimi*
+
* Fix `Encoding::CompatibilityError` when public path is UTF-8
In #5337 we forced the path encoding to ASCII-8BIT to prevent static file handling
View
3  actionpack/lib/action_dispatch/routing/mapper.rb
@@ -1436,7 +1436,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 Owner

Does this need to be there?

@amrnt
amrnt added a note

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 -\

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
options[:action] ||= action unless action.include?("/")
else
action = nil
@@ -1632,6 +1632,7 @@ def name_for_action(as, action) #:nodoc:
when :root
[name_prefix, collection_name, prefix]
else
+ prefix.gsub!(/\-/, '_') if prefix.is_a?(String)
[name_prefix, member_name, prefix]
end
View
4 actionpack/test/dispatch/routing/inspector_test.rb
@@ -37,6 +37,7 @@ def self.inspect
end
engine.routes.draw do
get '/cart', :to => 'cart#show'
+ get '/view-cart', :to => 'cart#show'
end
output = draw do
@@ -50,7 +51,8 @@ def self.inspect
" blog /blog Blog::Engine",
"",
"Routes for Blog::Engine:",
- " cart GET /cart(.:format) cart#show"
+ " cart GET /cart(.:format) cart#show",
+ "view_cart GET /view-cart(.:format) cart#show"
], output
end
View
2  actionpack/test/dispatch/routing/route_set_test.rb
@@ -30,10 +30,12 @@ def call(env)
draw do
get 'foo', to: SimpleApp.new('foo#index')
get 'bar', to: SimpleApp.new('bar#index')
+ get 'foo-bar', to: SimpleApp.new('bar#index')
end
assert_equal '/foo', url_helpers.foo_path
assert_equal '/bar', url_helpers.bar_path
+ assert_equal '/foo-bar', url_helpers.foo_bar_path
end
test "url helpers are updated when route is updated" do
Something went wrong with that request. Please try again.