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

Adds API.route_param for quick declaration of route parameters. #376

Merged
merged 2 commits into from
Apr 14, 2013

Conversation

mbleigh
Copy link
Contributor

@mbleigh mbleigh commented Mar 30, 2013

I've found myself often implementing this pattern:

namespace :users do
  namespace ':id', requirements: {id: /[0-9]+/} do
    get { }
    put { }
  end
end

And thought that it might be useful to implement a little bit of syntactical sugar for it:

namespace :users do
  route_param :id, requirements: /[0-9]+/ do
    get { }
    put { }
  end
end

Thoughts?

@dblock
Copy link
Member

dblock commented Mar 30, 2013

I find the name a bit confusing. Is it a route or a parameter? Or a route parameter? Maybe just param? Curious what others think.

# @param param [Symbol] The name of the parameter you wish to declare.
# @option options [Regexp] You may supply a regular expression that the declared parameter must meet.
def route_param(param, options = {}, &block)
options[:requirements] = {param.to_sym => options[:requirements]} if options[:requirements].is_a? Regexp
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you want to be modifying the hash that is being passed in.

namespace_options = {}
namespace_options.merge! options
namespace_options[:requirements] = ... if options[:requirements].is_a? Regexp
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Did a quick dup instead but same effect.

@mbleigh
Copy link
Contributor Author

mbleigh commented Apr 14, 2013

I thought about the name for a bit and couldn't think of a better one, though I'm open to suggestions. You are declaring a parameter in the route, but I can also see how it's slightly confusing.

param would be confusing since we already have the params method, param_segment doesn't work as well for me. Any other ideas?

@dblock dblock merged commit ec08264 into master Apr 14, 2013
@dblock
Copy link
Member

dblock commented Apr 14, 2013

Merged, updated CHANGELOG.

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

Successfully merging this pull request may close these issues.

2 participants