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

Routes not doing what I would like them to do. #618

Closed
philly-mac opened this Issue Aug 5, 2011 · 13 comments

Comments

Projects
None yet
4 participants
@philly-mac

philly-mac commented Aug 5, 2011

Hi,

I have a route like this in one of my controllers

get :reindex do

When I do a padrino rake routes I see

(:tracks, :reindex) GET /tracks/re

Why is the router just chopping off the word index in any context?

furthermore could you actually explain why index seems to be the only one of the methods that has a special meaning in the context of the router?

Why doesn't

delete :destroy do ...

automatically map to something like

(:tracks, :destroy) DELETE /tracks/:id

The whole thing just seems very inconsistent to me, and I have always wondered why it worked this way.

Thanks

@ghost ghost assigned joshbuddy Aug 5, 2011

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE Aug 5, 2011

Member

For the first question index is a reserved word,

@joshbuddy?

Member

DAddYE commented Aug 5, 2011

For the first question index is a reserved word,

@joshbuddy?

@philly-mac

This comment has been minimized.

Show comment
Hide comment
@philly-mac

philly-mac Aug 5, 2011

I understand that index may be reserved, but is reindex?
That would be like saying you couldn't use "definitely" as a ruby method because it contains the word def.

philly-mac commented Aug 5, 2011

I understand that index may be reserved, but is reindex?
That would be like saying you couldn't use "definitely" as a ruby method because it contains the word def.

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE Aug 5, 2011

Member

You are right, but we need to parse better to replace index with /, Im sure @joshbuddy will patch it... soon. :D

Member

DAddYE commented Aug 5, 2011

You are right, but we need to parse better to replace index with /, Im sure @joshbuddy will patch it... soon. :D

@philly-mac

This comment has been minimized.

Show comment
Hide comment
@philly-mac

philly-mac Aug 5, 2011

Cool.

Which part of the framework deals with this? I might take a look at it myself also

philly-mac commented Aug 5, 2011

Cool.

Which part of the framework deals with this? I might take a look at it myself also

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Aug 5, 2011

Member

@philly-mac Yeah I totally agree this needs to be fixed and perhaps rethought about. Here is the exact method/lines: https://github.com/padrino/padrino-framework/blob/master/padrino-core/lib/padrino-core/application/routing.rb#L588. Right now we are doing a regex that is clearly not working. We should add a failing test for an action called reindex.

I can see the case for "stripping" out any of the reserved actions from the path as you suggest. "destroy" action becomes: DELETE /resource/id, etc to stick with restful conventions. Perhaps the key is to support this with a separate plugin, I am not entirely sure. Perhaps we can brainstorm a "better" way for routing to work which can be enabled that makes all destroy,update,show,create,index all reserved words? @DAddYE what do you think about this idea.

Member

nesquena commented Aug 5, 2011

@philly-mac Yeah I totally agree this needs to be fixed and perhaps rethought about. Here is the exact method/lines: https://github.com/padrino/padrino-framework/blob/master/padrino-core/lib/padrino-core/application/routing.rb#L588. Right now we are doing a regex that is clearly not working. We should add a failing test for an action called reindex.

I can see the case for "stripping" out any of the reserved actions from the path as you suggest. "destroy" action becomes: DELETE /resource/id, etc to stick with restful conventions. Perhaps the key is to support this with a separate plugin, I am not entirely sure. Perhaps we can brainstorm a "better" way for routing to work which can be enabled that makes all destroy,update,show,create,index all reserved words? @DAddYE what do you think about this idea.

@philly-mac

This comment has been minimized.

Show comment
Hide comment
@philly-mac

philly-mac Aug 5, 2011

@nesquena A plugin would be a good solution I think, something that you could just require to get the restful type routes. I went back to actually look at the way sinatra was doing its routes, and I like them because they are simple and consistent, and sometimes I would just like to be able to use routes as defined by sinatra.

So I think a plugin should add that when any of the 7 methods are used in their proper context then they become the restful route.
i.e.

delete :destroy do... #=> DELETE /resource/:id
but
get :destroy do... #=> GET /resource/destroy

I think it only makes sense to reserve the rest methods for the http methods that they apply to, and not just globally.

Also the rails guides has a perfect table of all methods and what they should map to http://guides.rubyonrails.org/routing.html

What do you think?

philly-mac commented Aug 5, 2011

@nesquena A plugin would be a good solution I think, something that you could just require to get the restful type routes. I went back to actually look at the way sinatra was doing its routes, and I like them because they are simple and consistent, and sometimes I would just like to be able to use routes as defined by sinatra.

So I think a plugin should add that when any of the 7 methods are used in their proper context then they become the restful route.
i.e.

delete :destroy do... #=> DELETE /resource/:id
but
get :destroy do... #=> GET /resource/destroy

I think it only makes sense to reserve the rest methods for the http methods that they apply to, and not just globally.

Also the rails guides has a perfect table of all methods and what they should map to http://guides.rubyonrails.org/routing.html

What do you think?

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Aug 5, 2011

Member

You can use sinatra routing directly as well passing a string directly. The symbol notation is an optional addition. So you can still do:

get "/foo/bar" do

end

in Padrino. I do think a plugin or a flag to support the restful paths by default by stripping the various keywords when used in the right context is not a bad idea at all. I can see using it in various places throughout APIs.

Member

nesquena commented Aug 5, 2011

You can use sinatra routing directly as well passing a string directly. The symbol notation is an optional addition. So you can still do:

get "/foo/bar" do

end

in Padrino. I do think a plugin or a flag to support the restful paths by default by stripping the various keywords when used in the right context is not a bad idea at all. I can see using it in various places throughout APIs.

@philly-mac

This comment has been minimized.

Show comment
Hide comment
@philly-mac

philly-mac Aug 5, 2011

Ahhh okay, but with the string notation you do not get any routes displayed with the padrino rake routes command. It was kind of misleading.

philly-mac commented Aug 5, 2011

Ahhh okay, but with the string notation you do not get any routes displayed with the padrino rake routes command. It was kind of misleading.

@philly-mac

This comment has been minimized.

Show comment
Hide comment
@philly-mac

philly-mac Aug 5, 2011

Also I don't know if you actually mention in the docs anywhere that the symbol notation is Padrino and string notation will give you sinatra routes, but you should make that clear somewhere as it is actually very useful to know.

philly-mac commented Aug 5, 2011

Also I don't know if you actually mention in the docs anywhere that the symbol notation is Padrino and string notation will give you sinatra routes, but you should make that clear somewhere as it is actually very useful to know.

@philly-mac

This comment has been minimized.

Show comment
Hide comment
@philly-mac

philly-mac Aug 6, 2011

Strike my last comment, I just looked through the guides again, and it does mention the distinction of the symbols

philly-mac commented Aug 6, 2011

Strike my last comment, I just looked through the guides again, and it does mention the distinction of the symbols

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Aug 9, 2011

Member

Added a failing test above, i.e reindex action

Member

nesquena commented Aug 9, 2011

Added a failing test above, i.e reindex action

@joshbuddy

This comment has been minimized.

Show comment
Hide comment
@joshbuddy

joshbuddy Aug 9, 2011

Contributor

Yeah, I'm working on this.

Contributor

joshbuddy commented Aug 9, 2011

Yeah, I'm working on this.

@joshbuddy

This comment has been minimized.

Show comment
Hide comment
@joshbuddy

joshbuddy Aug 11, 2011

Contributor

Fixed in 790f349

Contributor

joshbuddy commented Aug 11, 2011

Fixed in 790f349

@joshbuddy joshbuddy closed this Aug 11, 2011

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