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

modify setup.py to reference requirements.txt #83

Merged
merged 3 commits into from
Jul 20, 2016
Merged

Conversation

jburos
Copy link
Member

@jburos jburos commented Jul 20, 2016

I modified setup.py to load install_requires from requirements.txt.

This should address situations like that seen in issue #69, which we thought we had fixed (by freezing isovar version) but it turned out we hadn't truly fixed -- we only changed isovar in requirements.txt but not in setup.py. Since requirements.ext was used to set up the travis testing environment, this failure to modify setup.py wasn't caught.

One downside of loading all packages from requirements.txt into setup.py is that we end up with utility packages like pylint included as requirements for cohorts. Same would go for packages required for testing, which may or may not be required for package function.

We could avoid this situation by having separate "package_requirements.txt" & "travis_requirements.txt" files, only one of which would be used to populate setup.py. But, maintaining separate files introduces the potential for the problem to re-occur.

Packages like pylint are hard-coded into the .travis-ci.yml and so could probably be taken out of this requirements.txt file entirely.

@jburos jburos assigned jburos and tavinathanson and unassigned jburos Jul 20, 2016
@tavinathanson
Copy link
Member

I'm not too worried about including pylint, nose, etc. We've even talked about always including the tests with the pip install as well. And it's not just for Travis, but anyone who wants to run tests/verify.

It might be cleaner to separate them out at some point, but for now this seems fine to me.

from setuptools import setup
import versioneer

here = path.abspath(path.dirname(__file__))
Copy link
Member

Choose a reason for hiding this comment

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

Duplicate logic/lines for grabbing the README path vs. the requirements.txt?

@coveralls
Copy link

coveralls commented Jul 20, 2016

Coverage Status

Coverage remained the same at 58.929% when pulling 673e594 on modify-setup into 71e0082 on master.

@@ -37,6 +40,12 @@
print("Failed to convert %s to reStructuredText", readme_filename)
pass

# get the dependencies and installs
with open(path.join(here, 'requirements.txt'), encoding='utf-8') as f:
Copy link
Member

Choose a reason for hiding this comment

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

Nit: double quotes to match the rest of the file?

@coveralls
Copy link

coveralls commented Jul 20, 2016

Coverage Status

Coverage remained the same at 58.929% when pulling 134185a on modify-setup into 71e0082 on master.

@tavinathanson
Copy link
Member

LGTM, feel free to merge when Travis passes

@jburos jburos merged commit 44a0059 into master Jul 20, 2016
@coveralls
Copy link

coveralls commented Jul 20, 2016

Coverage Status

Coverage remained the same at 58.929% when pulling 8df2e5c on modify-setup into 71e0082 on master.

@jburos jburos deleted the modify-setup branch July 20, 2016 22:04
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.

3 participants