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

Add redirect feature #166

Merged
merged 3 commits into from Apr 30, 2012
Merged

Add redirect feature #166

merged 3 commits into from Apr 30, 2012

Conversation

allenwei
Copy link
Contributor

No description provided.

@dblock
Copy link
Member

dblock commented Apr 17, 2012

I like it. You also need to document it in the README.

I would change redirect to take an optional code and default it to nil, which causes it to be a 302/303. People might want permanent redirects and stuff like that (304).

@allenwei
Copy link
Contributor Author

@dblock thanks. let me change it

@dblock
Copy link
Member

dblock commented Apr 22, 2012

I am going to nitpick, but since we're at it, do you think it's worth changing permanent = true to an options hash? So that I can do redirect url, permanent: true. It's more explicit and extensible, although I am struggling to find what we'll extend it to?

Leaving this open for a couple of days before merging! Speak up if you have an opinion.

@allenwei
Copy link
Contributor Author

@dblock I've considered it before, I think user will do permanent redirect in web server in most of the case, so I use false as default value.

@dblock
Copy link
Member

dblock commented Apr 25, 2012

I meant like this:

def redirect(url, options = {})
   # permanent redirect if options[:permanent]
end

That is if we think there will be more to options than permanent vs. non-permanent?

@simb
Copy link

simb commented Apr 30, 2012

This looks great! Lets get it merged :)

dblock added a commit that referenced this pull request Apr 30, 2012
@dblock dblock merged commit b13e123 into ruby-grape:master Apr 30, 2012
@dblock
Copy link
Member

dblock commented Apr 30, 2012

Merged, thank you for your cooperation.

@christopherlai christopherlai mentioned this pull request May 30, 2012
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.

None yet

3 participants