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

Allow setting a symbol as path in scope on routes #8114

Merged
merged 1 commit into from Nov 22, 2012

Conversation

guilleiguaran
Copy link
Member

Was surprising found that this example doesn't work:

scope :api do
  resources :users
end

and the right form to use it is:

scope 'api' do
  resources :users
end

I think this should work similary as namespace where both are allowed.
These two are equivalent:

namespace :api do
  resources :users
end
namespace 'api' do
  resources :users
end

@carlosantoniodasilva
Copy link
Member

I think it's fine to support symbols in this case, at least I can't see anything that should block it. A changelog entry would be nice :). Thanks!

/cc @pixeltrix @rafaelfranca

@@ -641,7 +641,7 @@ def scope(*args)
options = args.extract_options!
options = options.dup

options[:path] = args.first if args.first.is_a?(String)
options[:path] = args.first.to_s if args.first.is_a?(String) || args.first.is_a?(Symbol)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can just do:

options[:path] = args.first.to_s if args.any?

I don't see any situation where it's going cause a problem. In fact I'm wondering whether we should just use args.flatten.join('/') if args.any? that way you can do the following:

scope :api, :v1 do
  resources :products
end

wdyt?

Choose a reason for hiding this comment

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

I think it's fine to add that support 👍. Not sure @tenderlove has anything to say about it.

@luke-gru
Copy link
Contributor

luke-gru commented Nov 9, 2012

I'm not sure I agree with this change. To me, the namespace code is namespacing a url within a string literal, and never a parameter. For example

namespace "api" { }

namepaces things under /api

Scoping, on the other hand, can work with parameters and string literals, so when provided with a symbol, it's not completely obvious if it would be transformed to its equivalent string representation or if it would represent a parameter.

For example:

scope :api

and

scope ":api"

would do completely different things with this patch, which might confuse people.

@pixeltrix
Copy link
Contributor

@luke-gru so do you think it should represent a parameter scope, e.g: scope ':api' ? At the moment the first parameter is ignored if it's not a string so we should do something - the choices are:

  1. raise ArgumentError if first param isn't a string
  2. call to_s on the first parameter
  3. use strings as a static segment and symbols as dynamic segments and raise for anything else

@carlosantoniodasilva @guilleiguaran wdyt?

@luke-gru
Copy link
Contributor

luke-gru commented Nov 9, 2012

@pixeltrix I agree that those are the three choices, and 1. is probably not a good one. After giving it a little thought, maybe calling to_s is the best choice. I was just chiming in to say that I understand why the symbol might be unsupported in this case up to now, because the expected behavior is not completely obvious. And in general, I think it makes more sense to use strings as arguments to scope

@guilleiguaran
Copy link
Member Author

@pixeltrix agree with @luke-gru, looks like to_s is the best option in this case

@pixeltrix
Copy link
Contributor

@guilleiguaran can you update to pull request to use args.flatten.join('/') if args.any? and add a test and changelog entry as well, thanks.

@guilleiguaran
Copy link
Member Author

@pixeltrix done 😄


*Guillermo Iguaran*


Choose a reason for hiding this comment

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

✂️

Was surprising found that this example doesn't work:

  scope :api do
    resources :users
  end

and the right form to use it is:

  scope 'api' do
    resources :users
  end

I think this should work similary as `namespace` where both are allowed.
These two are equivalent:

  namespace :api do
    resources :users
  end

  namespace 'api' do
    resources :user
  end
pixeltrix added a commit that referenced this pull request Nov 22, 2012
Allow setting a symbol as path in scope on routes
@pixeltrix pixeltrix merged commit 9f68d52 into rails:master Nov 22, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants