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

[WIP] New route definitions #2003

Closed
wants to merge 14 commits into from
Closed

[WIP] New route definitions #2003

wants to merge 14 commits into from

Conversation

asvetlov
Copy link
Member

@asvetlov asvetlov commented Jun 22, 2017

Now we have methods like app.router.add_get() for filling route table.
It works pretty well but I see constant request for other approaches: url table in django/tornado style and decorators like bottle or django.

The PR proposes support these two ways.

Url table:

    app.router.add_routes([
        web.get('/path', handler1),
        web.post('/path', handler2),
    ])

.add_routes() appends table to the router.

Other approach is using decorators for web handlers in pyramid-style:

@web.get('/simple')
async def simple(request):
    return web.Response()

app.router.scan(__name__)

Decorator stores info into handler.__dict__ without adding it into router.
app.router.scan(__name__) scans module content and adds all registered routes.
app.router.scan(__package__) scans all imported modules from the package.
Technically it iterates over sys.modules and scans all modules started from given name in alphabetical order.

Why use .scan(name) instead of @app.router.get() decoration? Because it doesn't create application on import stage which is the very bad style I believe.

Decoration is maybe not good for big projects but very convenient for relative small code base at least.

P.S.
I used to hope somebody invent these approaches in third-party library but I see no success.
Maybe the concept is not very easy to implement. So let's do it ourself.

P.P.S.
Documentation is needed to be updated, sure.
Honestly I have no idea what route definition style should be proposed first (in index.rst and tutorial).

Opinions?

@fafhrd91
Copy link
Member

fafhrd91 commented Jun 22, 2017

for decorator style registrations, why not #1735 ?

as we talk about small projects, creating application object is not a problem. also #1735 can be used only in one module.

@asvetlov
Copy link
Member Author

As I said it requires application creation on import time.
Like

app = web.Application()

@app.route('get', '/path')
def handler(request):
     pass

It forces app to be global variable, I hate global vars.
Also I have a feeling what we will force application creation for coroutine only eventually.
I understand why app._init_loop() exists but it looks a bit hackish.
Most likely recommended asyncio style will be `asyncio.run_forever() (python/asyncio#465). This way discourages calling asyncio code from regular functions and module namespace.

@asvetlov
Copy link
Member Author

See also https://github.com/aio-libs/aiohttp/pull/1735/files#r107077906
My proposal is very similar from usage perspective but doesn't use global variable for storing route definitions. The global var might be a source of problems when used with multiple application objects (or main app and subapps).

@fafhrd91
Copy link
Member

replace Application with different object with limited responsibilities and that would simplify everything, no extra code for scanning, context is explicit, etc.

UserManagement = aiohttp.Routes()

@UserManagement.get('..')
def handle(req):
   pass

@UserManagement.post('..')
def create(req):
   pass

Views = ...

@Views.get()
def other(..):
   pass

web.Application().register_routes(View)
web.Application().register_routes(base='/management/', UserManagement)

it is even possible to register_routes with permissions, etc

everything pretty explicit.

@asvetlov
Copy link
Member Author

Hmm. Makes sense. I like web.Routes() concept.

But let's collect more opinions first.

@asvetlov asvetlov mentioned this pull request Jun 22, 2017
@justanr
Copy link

justanr commented Jun 22, 2017

I haven't gone over this in detail yet, but is this something like Flask blueprints? A delayed registration data store?

I'm -1 on scanning all over sys modules, as that seems like it'll inevitably cause collisions.

The code I developed at work uses a decorator to shove some info on the handler and then there's a helper that takes a single module and a router and yanks all the decoratored handles off and passes them to the router register method. But it was a little tricky to account for handlers that live at multiple endpoints

@asvetlov
Copy link
Member Author

No, Flask blueprint is not just a route table -- it has also own templates, signals and middlewares.
And yes, scanning over modules is too tricky.
#2004 solves the problem better I believe.

@justanr
Copy link

justanr commented Jun 23, 2017

I'm aware of that, but the most common usage is probably delayed routing registration. I hardly see extended usage of blueprints. Though I only know of a few OSS Flask projects where that sort of thing would be useful too. Places like Twilio probably make good use of them but that's purely conjecture.

@asvetlov
Copy link
Member Author

asvetlov commented Jun 23, 2017

@justanr it's done by PR:

    app.router.add_routes([
        web.get('/path', handler1),
        web.post('/path', handler2),
    ])

apistar.Include counterpart is not implemented yet but it could be done by several lines of code.

@asvetlov asvetlov added this to the 2.3.0 milestone Jun 23, 2017
@asvetlov asvetlov closed this Jul 2, 2017
@asvetlov asvetlov deleted the route_deco branch July 2, 2017 20:46
@lock
Copy link

lock bot commented Oct 28, 2019

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.

@lock lock bot added the outdated label Oct 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants