-
Notifications
You must be signed in to change notification settings - Fork 143
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
Cleanup setup.py - install_requires #149
Conversation
Per the Python packaging guidelines[1], setup.py should only contain the minimal requirements needed to run the code correctly. This commit removes the dev and test dependencies from setup.py, which should not be installed in production envs. [1]: https://packaging.python.org/discussions/install-requires-vs-requirements/
Hey @JshWright! Thank you for this PR. I need to pull this down and make sure we don't have any issues here, so it may be a few days before we can get this merged in. I believe this is an obvious fix as well, but since it does change functionality and you don't mind, go ahead and sign and send a CLA over to us. Ill keep you updated on this PR |
@@ -40,5 +34,16 @@ def get_requirements(): | |||
"License :: OSI Approved :: Apache Software License", | |||
"Topic :: Software Development :: Libraries :: Python Modules", | |||
], | |||
install_requires=get_requirements() | |||
install_requires=[ | |||
"asyncio", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR.
Do we actually require a dependency on asyncio lib?
Since Python 3.4 asyncio is moved to stdlib.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's a good call out. I only removed the really obvious testing related libraries, but that's likely not needed either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, looks like this change should resolve this issue - #157
Hi @JshWright. I've included your commit along with another one related into this #159 and just merged to master. Need to wait until we release v1.2.2. Thanks for your contribution. |
Thanks @serhiibuniak-okta, looking forward to 1.2.2! |
@JshWright we decided to include some additional changes, so we released v1.3.0. It is ready. Thanks again. |
Per the Python packaging guidelines1, setup.py should only contain the minimal
requirements needed to run the code correctly. This commit removes the dev and
test dependencies from setup.py, which should not be installed in production envs.
As this commit contains no functional changes to the okta-sdk-python library, I consider this an "obvious fix" for the purposes of the contributor guidelines, and therefore a CLA is not necessary (I'm happy to sign one if the maintainer feels differently though)