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

Add simple static server extension #16

Merged
merged 5 commits into from Nov 7, 2017

Conversation

2 participants
@yohanboniface
Member

yohanboniface commented Nov 3, 2017

No description provided.

@yohanboniface yohanboniface requested a review from davidbgk Nov 3, 2017

@@ -233,6 +231,7 @@ class Roll:
def __init__(self):
self.routes = self.Routes()
self.hooks = {}
from .extensions import options

This comment has been minimized.

@yohanboniface

yohanboniface Nov 3, 2017

Member

We are now importing HTTPError from extensions, so cyclic import :/
Should we move HTTPError to a third file?

This comment has been minimized.

@davidbgk

davidbgk Nov 3, 2017

Contributor

Or not apply options by default?

This comment has been minimized.

@yohanboniface

yohanboniface Nov 3, 2017

Member

Tempting!
But I'm afraid each and every Roll usage will need to use it, as I think a web server that does not answers to options seems ineffective.

This comment has been minimized.

@davidbgk

davidbgk Nov 3, 2017

Contributor

I think it's only in case of an API, Roll can just serve regular views fine without responding to OPTIONS requests(?)

This comment has been minimized.

@yohanboniface

yohanboniface Nov 3, 2017

Member

OK, let's try that way :)

prefix += '/'
prefix += '{path:path}'
async def serve(request, response, path):

This comment has been minimized.

@yohanboniface

yohanboniface Nov 3, 2017

Member

I wonder if this function should exist outside of static, so it can be used as a util (something like serve_from_file).
I've also been tempted to make a shortcut out of it, something like:

response.file = Path('path/to/file')

Which would do what serve currently does (raise if file does not exist, guess type, set header, etc.).
But I feel it's a bit too much job done to be put onto a shortcut.

Thoughts?

This comment has been minimized.

@davidbgk

davidbgk Nov 3, 2017

Contributor

I think using response.file directly would encourage to keep that in production.

What would be the correct way to transition to serving file from nginx? Maybe a good thing to add to the documentation?

This comment has been minimized.

@yohanboniface

yohanboniface Nov 3, 2017

Member

I think using response.file directly would encourage to keep that in production.

Agreed.

What would be the correct way to transition to serving file from nginx?

Just a directive that comes before Roll.

Maybe a good thing to add to the documentation?

Yes, I think the "deploy to production" part of the documentation needs more love. But I think it's outside of this given PR.

@@ -11,6 +11,12 @@ A changelog:
## dev version
* Changed `Roll.hook` signature to also accept kwargs.
* `json` shorcut sets `utf-8` charset in `Content-Type` header

This comment has been minimized.

@davidbgk

davidbgk Nov 7, 2017

Contributor

Add links to PRs? 🙃

@yohanboniface yohanboniface merged commit 8559b59 into master Nov 7, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@yohanboniface yohanboniface deleted the static branch Nov 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment