ciscorn / starlette-graphene3

An ASGI app for using Graphene v3 with Starlette / FastAPI
MIT License
101 stars 14 forks source link

Mention Dependency on python-multipart in README #27

Closed erikwrede closed 2 years ago

erikwrede commented 2 years ago

Fixes #25

ddelange commented 2 years ago

As this library provides multipart functionality, shouldn't the python-multipart library not be part of the requirements? (it's super lightweight anyway)

ddelange commented 2 years ago

or rather switch to starlette[full]? ref https://github.com/encode/starlette/blob/0.20.1/setup.py#L47

erikwrede commented 2 years ago

I wanted to keep this PR as small as possible, only reflecting similar mentions of python-multipart in the FastAPI and starlette docs. These are my reasons:

As this library provides multipart functionality, shouldn't the python-multipart library not be part of the requirements? (it's super lightweight anyway)

I don't disagree with you, but, since this is a starlette extension, I thought I'd stick with the opinion of the starlette maintainers for this PR, see https://github.com/encode/starlette/issues/445

or rather switch to starlette[full]? ref https://github.com/encode/starlette/blob/0.20.1/setup.py#L47

This is no longer a lightweight change IMO, I don't see a use case of having jinja2 included in a pure GQL server.

codecov[bot] commented 2 years ago

Codecov Report

Merging #27 (9889c11) into master (e18aa25) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #27   +/-   ##
=======================================
  Coverage   99.56%   99.56%           
=======================================
  Files           1        1           
  Lines         228      228           
=======================================
  Hits          227      227           
  Misses          1        1           
Flag Coverage Δ
unittests 99.56% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out 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 e18aa25...9889c11. Read the comment docs.

ciscorn commented 2 years ago

thanks!