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

Routing decorator #17

Closed

Conversation

jordaneremieff
Copy link
Contributor

Refs #12

Implements a method on the Router class to allow a simple means of routing ASGI applications to Path objects via a decorator.

Example:

from starlette import Router, Response


app = Router()


@app.route('/', methods=['GET'])
def homepage(scope):
    return Response('Hello, world', media_type='text/plain')

@tomchristie
Copy link
Member

Nice work. Tho I do want to think about this a bit more.

This routes to an ASGI application, so it's not quite the view style that lots of devs would expect:

@app.route(...)
async def do_something(request):
    ...

I think that's potentially confusing.

Related: Thinking on it, I also dislike our existing @asgi_application in that it changes the function signature, if anything we should document it up in a non-decorator way, eg:

async def homepage(request):
    ...
    return response

app = make_asgi(homepage)

(Still not sure about any of the names there)

@jordaneremieff
Copy link
Contributor Author

jordaneremieff commented Jul 12, 2018

I understand what you mean, I think fleshing out a more traditional view pattern in a pure-ASGI context would make things quite a bit more clear.

Two question I have at this point:

  1. Should we also make a more explicit distinction between application/protocol routing vs. url/path routing?

  2. Would it make sense to introduce a view pattern into Starlette, or would that be out of scope?

Because how I am understanding the current routing components seems consistent with how the decorator is being used here, and I would think any confusion using the decorator would be experienced when using the current router functionality also.

I could continue implementing this outside of Starlette and work the routing components into a more appropriate view pattern (eventually), however, I think the convenience of this routing style could be desirable to enough people to include here somehow.

@jordaneremieff
Copy link
Contributor Author

jordaneremieff commented Jul 12, 2018

Thinking about this a bit more, perhaps the asgi_application decorator could be the answer to my second question (re: introducing a view pattern)? I wasn't clear on its purpose initially and ignored it when I was working on this - maybe refactoring that method with routing in mind is the answer here, not sure.

@jordaneremieff
Copy link
Contributor Author

jordaneremieff commented Jul 15, 2018

Somewhat related to this:

I started on a new project using Starlette, and I'm currently trying to figure out a good way of handling the routing/templating/views. Below is my first attempt to come up with a CBV pattern using the functionality asgi_application provides.

import asyncio
from starlette import Request, Response
from starlette.routing import Router, Path
from starlette.types import Receive, Send, Scope


class View:
    def __init__(self, scope: Scope):
        self.scope = scope

    async def __call__(self, receive: Receive, send: Send) -> None:
        request = Request(self.scope, receive)
        request_method = request.method if request.method != "HEAD" else "GET"
        func = getattr(self, request_method.lower(), None)
        if func is None:
            raise Exception(
                f"Method {request_method} is not implemented for this view handler."
            )
        if asyncio.iscoroutinefunction(func):
            response = await func(request)
        else:
            response = func(request)
        await response(receive, send)


class HomepageView(View):
    # async def get(self, request):
    #     response = Response("Hello async world!", media_type="text/plain")
    #     return response

    def get(self, request):
        response = Response("Hello sync world!", media_type="text/plain")
        return response

app = Router([Path("/", app=HomepageView, methods=["GET"])])

It would need to be finessed a bit and the asgi_application logic could be pulled out of the view class itself and implemented using a make_asgi method in the way you described above - think something along the lines of Django's as_view().

Would something like this be in the scope of Starlette? If it is too much to include here, I could see about doing a PR for just the make_asgi bit and implement the view pattern in Afiqah.

@tomchristie
Copy link
Member

Exactly what I was thinking, yes. Perhaps @asgi_application could be renamed to ‘@view’ too.

@tomchristie
Copy link
Member

One other thing here: we could run sync views inside a threadpool, always.

For static views that just return some templates data or a fixed response, that’s just unnecessary overhead, but for most teal view cases it’d be what you want.

That’d allow you to use either standard thread-concurrency code at the view level, or asyncio-concurrency code instead if needed.

@jordaneremieff
Copy link
Contributor Author

jordaneremieff commented Jul 16, 2018

What are your thoughts on creating a distinction between application and view routing? Also about including class-based views? The answers in #3 made things generally clear, but there are still a few grey areas in regards to the scope of Starlette that are relevant here.

@tomchristie
Copy link
Member

What are your thoughts on creating a distinction between application and view routing?

I think we should only provide routing to ASGI applications. For class based views that works seemlessly, since View is an ASGI app.

For function based views, we’ve got a request/response interface tho. Either you can wrap that function up in a decorator to make it an ASGI app (That’s what we currently have documented, tho I don’t like that it changes the signature, makes the interface less directly testable) or instead wrap the adapter around the function within the routing, something like ‘Path(‘/blah’, app=asgi_application(my_view))’

@tomchristie
Copy link
Member

In other words: I think it’d make sense to start by adding CBVs, with our existing routing. I like decorator routing, but still not sure how to best apply it.

@jordaneremieff
Copy link
Contributor Author

Thanks for clarifying. I'll close this PR now and see about introducing CBVs first with the existing routing then approach the decorator routing later.

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.

2 participants