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

[Trio] Quick fixes to make Sanic usable on hypercorn -k trio myweb.app #1767

Merged
merged 4 commits into from Jan 20, 2020

Conversation

Tronic
Copy link
Member

@Tronic Tronic commented Jan 19, 2020

Quite surprisingly it only took a few lines of changes to make Sanic run on Trio (with Trio-using webapp), on top of Hypercorn. If it is that easy, I'd like to include this in mainline, provided that it can be configured in some smart way (the first commit lacks configuration).

Is there some way that the library could be chosen at module load time, to avoid importing aiofiles when running on trio?

@sjsadowski
Copy link
Contributor

We're going to need to update the CI tests for this, but I like where it's going. Could you add a quick section in docs/sanic/examples.rst for it?

@@ -3,7 +3,8 @@
from os import path
from urllib.parse import quote_plus

from aiofiles import open as open_async # type: ignore
#from aiofiles import open as open_async # type: ignore
from trio import open_file as open_async
Copy link
Member

Choose a reason for hiding this comment

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

The easy way'd be defining those incompatible imports in compat.py module, then import things from compat.

Copy link
Member Author

Choose a reason for hiding this comment

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

Somewhat better now but detection of environment by argv is far from optimal...

@codecov
Copy link

codecov bot commented Jan 20, 2020

Codecov Report

Merging #1767 into master will decrease coverage by 0.15%.
The diff coverage is 77.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1767      +/-   ##
==========================================
- Coverage   92.18%   92.03%   -0.16%     
==========================================
  Files          22       23       +1     
  Lines        2240     2273      +33     
  Branches      419      424       +5     
==========================================
+ Hits         2065     2092      +27     
- Misses        136      141       +5     
- Partials       39       40       +1
Impacted Files Coverage Δ
sanic/static.py 93.75% <100%> (ø) ⬆️
sanic/response.py 100% <100%> (ø) ⬆️
sanic/compat.py 69.23% <55.55%> (-30.77%) ⬇️
sanic/handlers.py 95.45% <0%> (-2.65%) ⬇️
sanic/exceptions.py 100% <0%> (ø) ⬆️
sanic/errorpages.py 100% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b565072...bb2bbe2. Read the comment docs.

@sjsadowski
Copy link
Contributor

@Tronic linter is complaining but everything else looks good now

@sjsadowski sjsadowski merged commit e908ca8 into sanic-org:master Jan 20, 2020
@sjsadowski
Copy link
Contributor

LGTM.

@Tronic
Copy link
Member Author

Tronic commented Jan 20, 2020

Okay. FWIW, this is a bit hacky and probably not 100 % functional/problem-free in Trio mode yet.

@Tronic Tronic deleted the quicktrio branch January 20, 2020 16:33
@sjsadowski
Copy link
Contributor

We'll just note in the docs that it's 'experimental support'

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.

None yet

3 participants