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

Added a custom server example using Flask #126

Closed
wants to merge 2 commits into from
Closed

Conversation

jrast
Copy link
Contributor

@jrast jrast commented Apr 1, 2019

See issue #125

@codecov
Copy link

codecov bot commented Apr 1, 2019

Codecov Report

Merging #126 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #126   +/-   ##
=======================================
  Coverage   93.88%   93.88%           
=======================================
  Files          60       60           
  Lines        2013     2013           
=======================================
  Hits         1890     1890           
  Misses        123      123

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 8e9cca7...e13d0a9. Read the comment docs.

docs/custom-server.rst Outdated Show resolved Hide resolved
docs/custom-server.rst Outdated Show resolved Hide resolved
docs/custom-server.rst Outdated Show resolved Hide resolved
@jrast
Copy link
Contributor Author

jrast commented Apr 10, 2019

Thanks for the review @tbsf
I will update the PR accordingly.

@jrast jrast force-pushed the patch-2 branch 2 times, most recently from ded937a to 7811f12 Compare April 10, 2019 22:37
@jrast
Copy link
Contributor Author

jrast commented Apr 10, 2019

Ok, I updated the example. It's working now with Ariadne 0.3.0 and includes the recomendations of @tbsf

@rafalp
Copy link
Contributor

rafalp commented Apr 11, 2019

I'm wondering if this shouldn't be 3rd party library, eg. flask_ariadne with readme, tests and Pypi release.

I'm not strongly opinionated but I'm worrying that we may be setting an expectation to the folk that our docs will eventually cover all Python web frameworks, and that's not going to work.

@drice
Copy link
Contributor

drice commented Apr 11, 2019

This is just my experience, but finding a Flask example was a deciding factor in giving this a try instead of sticking with Apollo. I didn't even notice there was also documentation for Django. Regardless of the route you choose to go down for documentation, I definitely suggest calling more attention to the ability to create a custom servers and mention that Django and Flask are supported in the README itself.

See Apollo's list of integrations for example: https://github.com/apollographql/apollo-server#installation-integrations

@rafalp
Copy link
Contributor

rafalp commented Apr 11, 2019

@tbsf this is excellent feedback, I haven't considered this. Thanks!

@jrast
Copy link
Contributor Author

jrast commented Apr 12, 2019

I don't see the need for a 3rd party library at the moment. It's only about 20 lines of code which are needed and a package for these lines seems to be an overkill and would remind me of some npm packages.

On the other side, I agree that it will be impossible to have a example for each web framework. And if a lot of framework specific examples are in the docs, issues about the framework integration will pop up which are difficult to solve if the maintainers are not working with these frameworks.
Maybe a note can be added in the docs that these examples are more a "Proof of Concept" and no support will be given?

@rafalp
Copy link
Contributor

rafalp commented Apr 12, 2019

I'll do the pragmatic thing here and assume I've been trying to optimize prematurely ;)

@jrast can you please update the PR to rename "custom servers" to "Integrations" and rename "Basic GraphQL server with ..." to "... example"? I know @patrys also wants to follow up with "Starlette example".

docs/custom-server.rst Outdated Show resolved Hide resolved
 - Renamed custom-servers to framework-integrations
 - Reworded the intro paragraph for framework integrations
 - Updated the titles
@rafalp
Copy link
Contributor

rafalp commented May 6, 2019

@jrast thank you for help! I'll close this PR and open my own that includes Flask chapter you've written as well some more generic pointers for other integration authors.

@rafalp rafalp closed this May 6, 2019
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

4 participants