search

mozilla-services / python-dockerflow

A Python package to implement tools and helpers for Mozilla Dockerflow
https://python-dockerflow.readthedocs.io
Mozilla Public License 2.0
38 stars 22 forks source link

Add Sanic support #27

Closed relud closed 5 years ago

relud commented 5 years ago

One thing I wondered though is the code overlaps with the Flask support, do you think we could/should abstract those away?

I don't think we should abstract them away. My experience switching from flask to sanic was that it's similar, but definitely not trying to be api compatible at all (many things slightly renamed). I think that having sanic and flask separate allows for properly supporting each in ways that make the most sense for them (such as adding await and async keywords, which I haven't figured out how to use yet, but I know is important)

jezdez commented 5 years ago

One thing I wondered though is the code overlaps with the Flask support, do you think we could/should abstract those away?

I don't think we should abstract them away. My experience switching from flask to sanic was that it's similar, but definitely not trying to be api compatible at all (many things slightly renamed). I think that having sanic and flask separate allows for properly supporting each in ways that make the most sense for them (such as adding await and async keywords, which I haven't figured out how to use yet, but I know is important)

Cool, that makes sense to me. Thanks for elaborating!

codecov-io commented 5 years ago

Codecov Report

Merging #27 into master will increase coverage by 0.09%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #27      +/-   ##
==========================================
+ Coverage   99.55%   99.65%   +0.09%     
==========================================
  Files          14       19       +5     
  Lines         451      572     +121     
  Branches       60       79      +19     
==========================================
+ Hits          449      570     +121     
  Misses          1        1              
  Partials        1        1
Impacted Files Coverage Δ
src/dockerflow/flask/app.py 98.52% <ø> (ø) :arrow_up:
src/dockerflow/sanic/__init__.py 100% <100%> (ø)
src/dockerflow/flask/checks/messages.py 100% <100%> (ø) :arrow_up:
src/dockerflow/flask/checks/__init__.py 100% <100%> (ø) :arrow_up:
src/dockerflow/sanic/app.py 100% <100%> (ø)
src/dockerflow/checks/messages.py 100% <100%> (ø)
src/dockerflow/checks/__init__.py 100% <100%> (ø)
src/dockerflow/sanic/checks.py 100% <100%> (ø)
... and 3 more

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 4b6b4cd...cd6e05d. Read the comment docs.

relud commented 5 years ago

@jezdez okay, this should be ready for a final review now

relud commented 5 years ago

Sorry but this needs another pass

no worries, my bad for misspeaking. s/final/another/

ready again.

relud commented 5 years ago

ready for another review.

I was messing with tests and I noticed that from dockerflow.flask.checks.messages import * created a dependency on flask, so I moved dockerflow.flask.checks.messages to dockerflow.checks.messages and then imported all of the values into the old location and threw a PendingDeprecationWarning, so dockerflow.sanic no longer depends on flask. While I was at it I added an extras_require for sanic.

relud commented 5 years ago

bump @jezdez