Skip to content

Optional dependencies for transports #158

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

Merged

Conversation

leszekhanusz
Copy link
Collaborator

See issue #147

@codecov-io
Copy link

codecov-io commented Oct 25, 2020

Codecov Report

Merging #158 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #158   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           14        14           
  Lines          941       941           
=========================================
  Hits           941       941           
Impacted Files Coverage Δ
gql/cli.py 100.00% <100.00%> (ø)

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 07cdaf4...6027a60. Read the comment docs.

@leszekhanusz leszekhanusz added the type: feature A new feature label Oct 25, 2020
@leszekhanusz
Copy link
Collaborator Author

Of course we'll need to document this before it is merged as it is a breaking change.

@leszekhanusz
Copy link
Collaborator Author

I'm starting to think that my PR #112 was actually a bad idea. We could still revert it as we are still in alpha?

@KingDarBoja
Copy link
Contributor

Seems to be the same setup as I did at graphql-server but I am curious about the usage of with supress statement in order to keep running the program if the user has not installed any "required" extras.

Regarding PR #112, well since you want to alert users to install required packages in order to use specific transports, the export should be keep on the same module scope, in other words, with this change it should avoid including all transports on global export (__init__.py).

@leszekhanusz
Copy link
Collaborator Author

Seems to be the same setup as I did at graphql-server

That's because I copied it from there 😆

@KingDarBoja
Copy link
Contributor

@leszekhanusz are you going to revert the #112 on this PR so it is two birds, one shot? 👓

@leszekhanusz
Copy link
Collaborator Author

@leszekhanusz are you going to revert the #112 on this PR so it is two birds, one shot?

Yes

@leszekhanusz
Copy link
Collaborator Author

I modified the tests so that you can run tests with only a single transport dependency.
To do that:

  • create a new python environment
  • Install the dependencies for a single transport
  • run the tests with the flag --aiohttp-only, --websockets-only or --requests-only depending on the selected dependency

to install test dependencies for only websockets for example:

pip install .[websockets,test_no_transport]

@leszekhanusz
Copy link
Collaborator Author

Now I still need to add GitHub actions to test this automatically

Copy link
Contributor

@KingDarBoja KingDarBoja left a comment

Choose a reason for hiding this comment

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

Awesome 🚀

@leszekhanusz leszekhanusz merged commit b42fe08 into graphql-python:master Nov 1, 2020
@leszekhanusz leszekhanusz deleted the feature_optional_dependencies branch November 1, 2020 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants