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

Should Router be a subclass of Starlette? #122

Closed
berislavlopac opened this issue Oct 17, 2018 · 3 comments
Closed

Should Router be a subclass of Starlette? #122

berislavlopac opened this issue Oct 17, 2018 · 3 comments

Comments

@berislavlopac
Copy link

berislavlopac commented Oct 17, 2018

Right now, Router is simply an ASGI app, which means that it can be used as a top-level app; however, it can't benefit from Starlette-specific features such as debug, lifespan events or exception handlers.

Would it make sense to make Router a subclass of Starlette? That way instead of:

app = Starlette()
app.mount('/', Router((
    Path('product/{product_id}', app=ProductEndpoint),
    Path('product', app=NewProductEndpoint),
    Path('products', app=ProductListEndpoint),
)))

we can have

app = Router((
    Path('product/{product_id}', app=ProductEndpoint),
    Path('product', app=NewProductEndpoint),
    Path('products', app=ProductListEndpoint),
))

and keep all the advantages of Starlette.

This is just a simplified example, of course.

@marcosschroh
Copy link
Contributor

I was wondering something similar or maybe when we instantiate Starlette we can set the router attribute as well, like we are doing with the debug attribute, but I am.not sure. What do you think guys?

@tomchristie
Copy link
Member

Would it make sense to make Router a subclass of Starlette

Not really, the Router class is a subset of the features that Starlette class offers.
If you want to mount a

What we should do tho, is make sure that router and app share the same fundamental API for routing. First pass on that means that stuff like this ought to just work.

app = Router()

@app.route('/')
async def homepage(...):
    ...

It's possible that we should hide away the lower-level Path interface (at least at this point), or else find a clean way to expose the same thing in Starlette, so that the two interfaces match.

Once we've got two properly matching interfaces, then it's clearer.

It's also worth highlighting that you can submount Starlette instances onto a parent, too. That's what you need to do if you want to have a submounted set of functionality that also handles eg. lifespan, or custom exception handling.

@berislavlopac
Copy link
Author

Thank you @tomchristie -- just a brief addition. I have actually expressed this wrong, as by "subclass" I was actually thinking of a "subtype", i.e. -- exactly as you said -- that it would be useful if all ASGI apps in Starlette (routes, endpoints etc) shared the same interface. Perhaps that can be formalised in the form of some kind of ABC or mixin or something similar. Thank you for the great work!

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

No branches or pull requests

3 participants