Skip to content

Conversation

radekwlsk
Copy link
Contributor

As #1110 correctly states, currently examples package is installed alongside graphene which is probably not intended as "examples" is listed in packages=find_packages(exclude=[...]) list.

I was able to fix it by changing examples in:
packages=find_packages(exclude=["tests", "tests.*", "examples"])
to examples*:
packages=find_packages(exclude=["tests", "tests.*", "examples*"])

Unfortunately there seems to be no easy way to make package examples an optional dependency when it is in the same repository. Maybe it will be a good idea to separate examples from main package and then it could be added as extras_require={"examples": ["graphene-examples"]}?

Is it even necessary to allow installation of examples or are they here only for documentation purposes?

Additional question:
I am also not sure what "tests" in exclude is supposed to do as all test files are currently also included in package installation.

* examples package will not be installed with graphene
Copy link
Member

@jkimbo jkimbo left a comment

Choose a reason for hiding this comment

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

This looks good @radekwlsk thanks. I think it's fine to not include the examples in the distribution.

If you can find a way to ignore all the test folders as well that would be great!

@jkimbo jkimbo merged commit 133a831 into graphql-python:master Apr 17, 2020
@radekwlsk
Copy link
Contributor Author

@jkimbo Thanks for merging it so fast! Glad to make my first OSS contribution 🔥

I will try to get on tests soon, it is possible that adding * to tests in excluded list will also do the trick.

@radekwlsk
Copy link
Contributor Author

radekwlsk commented Apr 26, 2020

@jkimbo After further investigating the tests packages in installation I've found that installing Graphene from source with

$ pip install .

or from PyPI with

$ pip install graphene

both include tests.
But when first building a source distribution and the installing from it with

$ python setup.py sdist
$ pip install dist/graphene-3.0b1.tar.gz

then the tests packages are indeed excluded even with current setup config (EDIT: which may be also caused by MANIFEST.in and not setup.py config directly).

Maybe the problem lies in how the Graphene is distributed through PyPI, not with the config?

@jkimbo
Copy link
Member

jkimbo commented Apr 26, 2020

That is possible @radekwlsk . Also I'm not sure that excluding tests is a good idea actually because there are some cases where you would want to run the tests from the distribution. See graphql-python/graphene-django#820

Do you have any objections to keeping the tests included as part of the release?

@radekwlsk
Copy link
Contributor Author

radekwlsk commented Apr 26, 2020

Do you have any objections to keeping the tests included as part of the release?

@jkimbo None at all, I've just figured that was the intention behind find_packages(exclude=["tests", "tests.*", ...]). I am well ok with that.

@jkimbo
Copy link
Member

jkimbo commented Apr 26, 2020

Yep I think that was the original intention but it seems that it's useful for some cases. I'll remove it from the exclude list.

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.

2 participants