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

Implement CBV pattern #35

Closed
wants to merge 7 commits into from

Conversation

jordaneremieff
Copy link
Contributor

@jordaneremieff jordaneremieff commented Jul 30, 2018

Refs #27

This is an attempt to define a CBV pattern in Starlette.

Example usage:

from starlette import PlainTextResponse
from starlette.app import App
from starlette.views import View


app = App()


class HomepageView(View):
    async def get(self, request, **kwargs):
        response = PlainTextResponse(f"Hello, world!")
        return response


class UserView(View):
    async def get(self, request, **kwargs):
        username = kwargs.get("username")
        response = PlainTextResponse(f"Hello, {username}")
        return response


app.add_route("/", HomepageView())
app.add_route("/user/{username}", UserView())

@jordaneremieff jordaneremieff mentioned this pull request Aug 1, 2018
from starlette.types import ASGIInstance, Receive, Send, Scope


def asgi_application(func):
def make_asgi(func):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm kinda thinking that we'll drop the asgi_application/make_asgi decorator altogether - I think it just muddies things up.

Let's leave this alone for now, but perhaps don't use it in the PR, and include any logic directly in the class.

def as_view(cls) -> ASGIApp:
def view(scope: Scope):
self = cls()
return self.dispatch(scope)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to do the same closure .as_view() dance that Django does?

Rather than using .as_view() I figure that we just implement the endpoint/view/whatever class as an ASGI application. We're not using multi-threading so I don't think we need the same "put stuff in a closure for safety" dance, but maybe I'm not thinking it through enough.

@jordaneremieff
Copy link
Contributor Author

I made some changes based on your comments - removed the as_view() and make_asgi methods from the view class. Still a bit more to do here when I have time, but those changes were simple enough to push for now.

Copy link
Member

@tomchristie tomchristie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’d also suggest we drop support for the handler methods being non-awaitable. Let’s just cover async stuff in Starlette.

@jordaneremieff jordaneremieff changed the title Implement CBV pattern and add method to router Implement CBV pattern Aug 29, 2018
@jordaneremieff
Copy link
Contributor Author

jordaneremieff commented Aug 29, 2018

I've made the following updates to this PR:

  • View class now only supports async methods
  • Returns '404 Not found' if an allowed method is unimplemented on the view class rather than raising exception
  • Intended to be used with App() class add_route method, previousadd_route method on Router removed
  • Documentation with brief description and example code added
  • Tests added

@tomchristie
Copy link
Member

Coolio!

One observation: We’ll want to make sure that each request gets a new instance, rather than routing to an instance of the class.

Also, use 406 for the method-does-not-exist-on-this-class case.

Good stuff!

@tomchristie tomchristie mentioned this pull request Aug 30, 2018
@tomchristie
Copy link
Member

Added some further commits on top of this, in #52. Thanks so much for this!

@delijati
Copy link

delijati commented Jul 5, 2019

hi @tomchristie can this be also added to the docs ... i found only this here

@foxmask
Copy link
Contributor

foxmask commented Jul 5, 2019

@delijati that was the case before the doc changed 0.12.0...master

@tomchristie
Copy link
Member

@foxmask @delijati https://www.starlette.io/endpoints/ (Or are we talking about something else?)

@foxmask
Copy link
Contributor

foxmask commented Jul 5, 2019

@tomchristie if that were endpoints here
0.12.0...master#diff-1a523bd9fa0dbf998008b37579210e12
then yes ;)

edit the pasted URL does not work fine so ... I wanted to show you :

0.12.0...master

then > "file changed" then we can see the docs/index.md

@tomchristie
Copy link
Member

We're not documenting a pure ASGI class, nope. We don't really need to in ASGI 3, since it's easier to write them as plain functions.

What we are documenting is the class-based HTTPEndpoint and WebSocketEndpoint implementations, which is actually what you want if you're looking for an CBV implementation.

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.

4 participants