routing subsystem refactoring

[fafhrd91]

routing subsystem refactoring

1. Route object is responsible for path only (endpoint)
2. handler moved to View object. we can add more matching checks to View, like specific headers, cookies, etc. right now we match only on method.
3. added system routes that match HTTPException, so we can register view for specific system route, i.e. HTTPNotFound, or HTTPForbidden, etc.

this is first version, for discussion
it is not backward compatible, we can make it backward compatible later when we decide if we want to use this changes.

@asvetlov @kxepal thoughts, ides, etc

[kxepal]

Looks like unrelated change. PURGE is used for Varnish needs, but what's it purpose for aiohttp?

[kxepal]

To not write "ok" for all inline comments, will say that here (:
Looks good for me, at least after reading. Will try to play tomorrow with the fresh head, but I think that's a good step forward with the large room for the improvements.

Also, I didn't get how to register system routes (tests also missed example of such case), but may be I should take a look a bit closer.

[fafhrd91]

System routes are not clear yet. At least Should we allow to replace existing system routes or just allow to set view on it.

[kxepal]

@fafhrd91 hm...I think that exceptions are the same base for views as methods:

route = router.add_route('foo', '/bar/baz')
route.add_view(METH_GET, handle_get)
route.add_view(HTTPNotFound, handle_404)
route.add_view(HTTPBadRequest, handle_400)
...

While there are also "default" system routes (actually views) to catch and process these errors when user route doesn't provides own handlers or accidentally reraise the error from own handler.

[fafhrd91]

Oh! I really like it.

[asvetlov]

I like idea very much but please don't rush with merging.
Let me review carefully.

[fafhrd91]

sure, it is too early for merging.

some ideas.
we can method default to METH_ANY and make it optional.
also we can extract path leaf and use it as view name, something like /path-to-comething/index.html becomes route with path /path-to-comething/ and view with name index.html

would be cool to make this to work route.add_view(HTTPBadRequest, handle_400), we may need context for view registration, but context adds so many complications.

[asvetlov]

I'm also thinking about nested applications and routers.

[tark-hidden]

Would you think about routing system based on route name? I mean...

app.add_route('foo', '/bar/baz')
app.add_view('foo', METH_GET, handler)
app.add_view('foo', METH_POST, handler)

...where foo is route_name. Yes, Pyramid-like, but... Pros: less variables, simpler code, easy to write. Cons: None.

[kxepal]

@tark-hidden This PR allows that, though you'll have to specify route name explicitly via keyword argument:

app.add_route('foo', '/bar/baz')
app.add_view(METH_GET, handler, route='foo')
app.add_view(METH_POST, handler, route='foo')

However, if route name is really mandatory for add_view method, indeed, why not to pass it with the first argument?

[tark-hidden]

Ah, sorry! I've read an example code with route = router.add_route('foo', '/bar/baz') and route.add_view(...). If I can use route variable as route name without unnecessary variables, it is fine! It that case placement of this parameter doesn't matter, and method as first parameter is better choice.

[fafhrd91]

@asvetlov did you have a chance to review this PR

[asvetlov]

Sorry, I still have no time for exhausting review. I'm busy with preparing to europython. Maybe next week?

[asvetlov]

As I see new route system is not compatible with existing one, right?

So maybe better to add new router class (UrlDispather2/web_urldispatcher2.py)?

I support splitting into route/view BTW.

[auvipy]

@fafhrd91 This branch has conflicts that must be resolved

[fafhrd91]

I do not have time for this change. Anyone wants to work on this change?

[auvipy]

count me on for this then

[fafhrd91]

Please coordinate this effort with @asvetlov

Sent from my iPhone

On Dec 7, 2015, at 8:20 AM, Asif Saifuddin Auvi [email protected] wrote:

count me on for this then

—
Reply to this email directly or view it on GitHub.

[auvipy]

hi @asvetlov

[asvetlov]

Hello.
The PR is not backward compatible with existing code.
There are two options:

1. Make it compatible (preferable solution)
2. Move new solution into new UrlDispatcher2 class with deprecating UrlDispatcher in future aiohttp releases (adds a burden of supporting two routers in parallel for a while, about 9-12 months long, which is really maintenance nightmare).

[fafhrd91]

i think we should maintain compatibility.

[auvipy]

i'm up for point 1

[asvetlov]

@auvipy please let me know when you will have something to discuss.

[auvipy]

@asvetlov at gitter? or mail or here?

[asvetlov]

I think github is better tool.
We will left a discussion history at least.

[auvipy]

ok let me know what u think should be done? I am up for adding a django like deprication utlis module for better management of feature deprication and related stuffs @ aiohttp an then add/depreicate features according to roadmape design plans.

[asvetlov]

Try to figure out first what changes are required for adding new functionality without breaking any existing test.

[auvipy]

OK

[asvetlov]

Done by #727

[lock]

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.