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

CHORE: consolidate requirements to setup.py #105

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

theY4Kman
Copy link
Contributor

Note: this is just a suggestion — I have no horse in this race

Description

This PR moves package requirements out of requirements.txt and straight into install_requires of setup.py (leaving . as the only dep in requirements.txt — which informs pip to look in the setup.py).

Additionally, deps for test_requires were moved out of requirements-dev.txt and into extras_require['test'], leaving halo[test] as the only dep in test_requires.

A new extras_require['dev'] was added, requiring halo[test,ipython], to setup the dev environment. .[dev] is now the only dep in requirements-dev.txt. Requiring the ipython reqs is a new addition to the workflow — it was added, because the linter complains if it can't import ipython stuff.

Why?

All I wanted to do was add -r requirements.txt to requirements-dev.txt, so setting up the dev env would only need pip install -r requirements-dev.txt... then I realized the reqs were read line-by-line in setup.py, so I could either augment the dependencies(req_file_path) method, or ask O Glorious Internet for answers.

Her Majesty, The Internet said "maybe use extras_require['test']". I'd never heard of going that route before (or that pip supports installation of reqs from setup.py using . as a dep), it seemed fun and interesting (for some definitions of "fun" and "interesting"), uhh, so I gave it a shot.

Checklist

  • Your branch is up-to-date with the base branch
  • You've included at least one test if this is a new feature
  • All tests are passing

@theY4Kman theY4Kman force-pushed the chore/dev/single-reqs-install-for-dev branch from d09f9ac to e7f1119 Compare October 27, 2018 20:05
@@ -1,14 +1,10 @@
from setuptools import setup, find_packages # pylint: disable=no-name-in-module,import-error


def dependencies(file):
with open(file) as f:
return f.read().splitlines()
Copy link
Contributor Author

@theY4Kman theY4Kman Oct 27, 2018

Choose a reason for hiding this comment

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

btw, this is equivalent to list(f) or f.readlines()

@coveralls
Copy link

coveralls commented Oct 27, 2018

Pull Request Test Coverage Report for Build 308

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 92.157%

Totals Coverage Status
Change from base Build 304: 0.0%
Covered Lines: 272
Relevant Lines: 289

💛 - Coveralls

@manrajgrover manrajgrover self-requested a review October 28, 2018 19:05
Copy link
Owner

@manrajgrover manrajgrover left a comment

Choose a reason for hiding this comment

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

@theY4Kman Left few comments which might need attention.

$ cd halo
$ pip install -e .
$ pip install -e requirements-dev.txt
Copy link
Owner

Choose a reason for hiding this comment

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

We can do pip install -e .[dev] here?

setup(
name='halo',
packages=find_packages(exclude=('tests', 'examples')),
include_package_data=True,
Copy link
Owner

Choose a reason for hiding this comment

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

@theY4Kman What does this line do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol, I'm not sure, tbh. The line was there previously, just below the reqs. I thought it looked out of place, so I moved it to the other line that said "package" :P

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, just noticed! I should check but I remember stumbling upon an error before adding it.

termcolor==1.1.0
colorama==0.3.9
six==1.11.0
--editable .
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think we need this file now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aye, I'd concur with this and your other comments. I think I kept 'em in for historical reasons, or to make as few procedural changes as possible. I'm not really sure why. But I do agree — I'll remove them.

nose==1.3.7
pylint==1.7.2
tox==2.8.2
--editable .[dev]
Copy link
Owner

Choose a reason for hiding this comment

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

Same here, is this file needed?

@manrajgrover
Copy link
Owner

@theY4Kman We should also remove requirements file from MANIFEST.in

Comment on lines +31 to +36
'log_symbols==0.0.11',
'spinners==0.0.23',
'cursor==1.2.0',
'termcolor==1.1.0',
'colorama==0.3.9',
'six==1.11.0',

Choose a reason for hiding this comment

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

Since halo is a library intended to be used by other scripts, it should be liberal in its requirements.
Not pin a version at all, or use '>=' to specify a known minimal version halo needs to function properly.

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.

4 participants