Major routing syntax cleanup #747
Comments
|
@achiu @DAddYE @skade I know this is a big change in some respects (favoring more explicitness) but I think this removes a lot of weird 'magic' and ambiguity in our routing. A new typical controller would look like: MyApp.controller :posts, "/users/:user_id/posts" do
before :index, :create do
# ...
end
get :index, "/" do
# /posts/
end
get :show, ":id" do
# /posts/:id
end
get :new, "new" do
end
post :create, "/" do
# /posts
end
get :edit, ":id/edit" do
end
put :update, ":id" do
# /posts/:id
end
delete :destroy, ":id" do
# posts/:id
end
end |
|
yes this looks very dry and also much more RESTful. |
|
Given the number of times I've tripped over the different keywords, I can only back this. The keywords where mostly inexpressive and with the new syntax I get the feeling that looking at a controller I might actually see my routes. :-) +1 |
|
Yeah that's the way we saw it too. I think the number of keywords and the inconsistent behavior really only served to confuse |
|
Love it! +1, I prefer that @joshbuddy make it inside padrino instead of making patch/adapters. |
|
I agree with DAddYE on this one, as having as much as possibly tightly knitted to the other parts does indeed improve performance and readability. Once (I don't know if it still is like that) you could define routes in Rails, where if the name of the route was :show and it wasn't mapped to a specific URI, it'd simply map it as /whatevers/:id, and not like Padrino does now, which is map it to /whatevers/:id/show. I would like to request this feature in padrino as well, unless of course it has been removed since, for a valid reason. |
|
+1 :D |
|
+1 for this. Every time I upgrade padrino I have problems with routes that start working in an inconsistent way. |
|
I would enjoy if there was no special meaning to route keys, but a way to configure those. For example: And somewhere else: |
|
@nesquena I like it, can we start to write specs? |
|
Adding wiki to document changes in next major point release: https://github.com/padrino/padrino-framework/wiki/Changes-for-Major-Release. Before releasing we need to document these breaking changes. |
|
Maybe I can help and contibute these mayor changes in the documentation page of padrino. |
|
Guys, why can't it be something as simple as: Typing all those route strings for each action doesn't feel dry, especially for the routes like 'get :edit, ":id/edit"'. Ideally it can be something like |
|
get :edit, :id # => "/:id/edit" |
|
Yep, I agree that the |
|
Fully agree with you. Routes should be simple to define in the first place in a REST based app. |
|
So whats your idea for overloading? As I said, I would prefer some preconfigured, but flexible way to attach default routes to labels. For example: And in the end, edit is an oddball in REST anyways, as it is only needed for browsers, so you are pretty free. You could argue about that one for ages. |
|
Correct, edit is a special representation of the resource. One could argue that not only browsers but other apps could use this representation as well. Say for instance a fat client app that builds its edit form by using the /:id/edit.xml representation. |
|
@skade, about attaching routes to labels — that's definitely a good idea, but when you attach a route to a label than it will also make sense to attach a verb to the same label (eg But the resulting API ( |
|
@mitya Padrino already uses the first argument as a label for generation, so it would not have to be build on top of it in any case. |
|
@nesquena +1 from me. I agree that the current routes are a bit too magical and doesn't feel right. |
|
Aren't you changing the behavior of This is the same syntax as |
|
Butt howto include the "/" character in a parameter? like this: get :tree, "/:branch/:folder(.*$)"
get :tree, :with => [:branch, :folder => /.*$/]http://my.com/tree/master/lib/good/folder params[:branch] = 'master'
params[:folder] = '/lib/good/folder'It''s not supported yet. And this is not supported too: get %r{/post/(\d\d)-(\d\d)-(\d\d\d\d)} do|month,day,year|
"Post requested from #{month}/#{day} in #{year}"
endCurrent the only way is: get %r{/post/(\d\d)-(\d\d)-(\d\d\d\d)} do
month= params[:captures][0]
day= params[:captures][1]
year= params[:captures][2]
end |
|
btw, the Regular Expression named parameters can use too: get %r{/post/(?<month>\d\d)-(?<day>\d\d)-(?<year>\d\d\d\d)}, :name => 'post', :generate_with =>'/post/' do
"Post requested from #{params[:month]}/#{params[:day]} in #{params[:year]}"
endAnd raise the "padrino rake routes" error |
|
ok, Got it in http_router: get :index, '/tree/:branch/:dir', :dir => /.*/ do |
|
Also see #820 for a common problem: should we allow routes with the same name, but different signatures? |
|
If we're going to re-work routes, what do people think of making controllers classes instead of blocks? One of the big problems I have at the moment is that sharing functionality between routes is awkward. Right now I have a controller helper that has generic helper methods for controllers, but these are shared between all routes and controllers, so it turns into a big mess of seemingly unrelated methods. With a controller class, you can define shared functionality as methods on the class. You also get inheritance, so like with my latest project, two controller that are very similar can share the majority of their code, with routes being overloaded only as needed. I just think class-based controllers give the user a lot more power without the awkwardness of the current controller block. You'd still retain all of the current methods, e.g. you'd still have #get, #post, #put, etc, and access to the everything routes normally have access to. You're simply wrapping them in the construct of a class (which is instanced when a request comes in), instead of a block. Thoughts? |
|
+1 @Wardrop specially inheritance seems like good idea. |
|
@pepe I've actually since started writing my own sinatra/padrino-inspired framework as we speak. It's in it's early stages of implementation, but I've already spent a lot of time playing with and refining the API and core constructs. Class-based controllers which respond to #call are the main construct; every object in the stack is rack-compliant, even routes expect a rack environment hash. Everything is meant to plug and play. I'm really happy with how it's shaping up. Sinatra itself is in need of a re-write which would suggest Padrino is probably equally so. I've been playing around with Sinatra, Padrino and Rack enough over the last 2 years to have a pretty good idea of what I want a ruby web framework to be, which is what Scorched will reflect. Thoughtful design is the projects #1 priority. For a little more info, checkout the repo I only just created: https://github.com/Wardrop/scorched |
|
Here another nightly work: https://github.com/padrino/padrino-framework/blob/new_core/examples/include.rb |
|
Another update: now also templates can work alone. See: https://github.com/padrino/padrino-framework/blob/new_core/examples/isolated_template.rb |
|
I'm all for it. 👍 |
|
OK looks like this is getting pushed to 0.12.0 (probably 1.0) |
|
Should we keep this issue open @padrino/core-members? Will any of this become part of the router reimplementation? Linking to mustermann for us to keep an eye on it. |
|
Yeah lets leave this open for now? it documents what we want to keep for our new routing syntax. |
|
@namusyaka @ujifgc we don't have to follow the proposal here exactly, but I'd like to take 0.13.0 as an opportunity for us to rethink our routing APIs in ways that might be some breaking changes. Examples of how we can simplify are in this thread. We can discuss here or elsewhere as we implement in the brand new routing engine. /cc @padrino/core-members |
|
I agree on the simplifying. |
|
Yeah, agreed we can push some of the syntactic routing sugar to a contrib. In particular, I like the following ideas for the new core syntax:
for routes that look like: Example::App.namespace :posts, "/users/:user_id/posts" do
before :index, :create do
# ...
end
get :index, "/" do
# /posts/
end
get :show, ":id" do
# /posts/:id
end
get :new, "new" do
end
post :create, "/" do
# /posts
end
get :edit, ":id/edit" do
end
put :update, ":id" do
# /posts/:id
end
delete :destroy, ":id" do
# posts/:id
end
endbut definitely open to suggestions from you all if you'd prefer an alternate. This would involve changing the name everywhere i.e |
|
Although something like this: # url(:posts, :index)
Example::App.namespace :posts, "/posts" do
desc "Homepage of my blog"
action :index
get "/" do
# => /here/my-blog
# link_to 'click', path.blog
end
endis kind of cool since it gets us back to the sinatra roots in a way. Interested to hear everyones thoughts. |
|
Even though the first approach is more consistent. I reckon that adding On that I'd probably add descriptors to parameters that your route accepts too and probably the return type? I know that probably makes its more complex but I think that all of these should be optional and inferred when possible. Thoughts? |
|
A thought: put that dsly thing into an optional plugin that can be |
|
I am using desc and parameters constrains in the https://github.com/intridea/grape and it works for me there. It is much more API oriented tho. +1 for namespace and REST thing. |
|
👍 Should we implement it after merging the new router? /cc @padrino/core-members |
|
I'm on for it :)
|
Josh has some ideas for some fairly major routing API changes to cleanup the whole controller situation. The core of this is that there are a number of ambiguous or confusing options in Padrino right now and more then that http_router has caused us some issues and has required a lot of ugly monkeypatching.
Josh argued what if instead we just go back to using (and augmenting) sinatra's router rather then requiring http_router. More then that we can simplify the routing syntax.
This approach changes the following things:
Below is a list of breaking api changes:
Basic Named Routes
The new syntax for named routes would be:
with the named route first and the url path second. Standard sinatra syntax is also supported.
Map Keyword
The
mapwould be deprecated:and changed to:
The
withkeywordwould be deprecated in favor of the more explicit:
get :account, "/:id" doControllers
Controllers would still act as a natural grouping for routes and would be prefixed into the route paths:
would still work (and append "/admin" to all contained routes), this would also work:
which would change the prefix in a way consistent with the new mapping syntax.
also still works.
The
parentkeywordwould be more explicit:
The
prioritykeywordWe would change priority to be for controllers rather than routes, i.e
get :show, :map => '/*page', :priority => :low dobecomes
MyApp.controllers :products, :priority => :lowThe text was updated successfully, but these errors were encountered: