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

Don't import biopandas during install #60

Merged
merged 5 commits into from
Oct 10, 2019
Merged

Don't import biopandas during install #60

merged 5 commits into from
Oct 10, 2019

Conversation

ecederstrand
Copy link
Contributor

Getting the version during install is more safely done by reading init as text.

The reason is that init may at some point include code that requires importing packages that have not yet been installed. Installing a package should always be possible using pip install biopython in an empty virtualenv.

Description

Insert Description Here

Related issues or pull requests

Link related issues/pull requests here

Pull Request requirements

For new features or bug fixes, pleas consider the following to-do list:

  • Added appropriate unit test functions in the ./biopandas/*/tests directories
  • Ran nosetests ./biopandas -sv and make sure that all unit tests pass
  • Checked the test coverage by running nosetests ./biopandas --with-coverage
  • Checked for style issues by running flake8 ./biopandas
  • Added a note about the modification or contribution to the ./docs/sources/CHANGELOG.md` file
  • Modified documentation in the appropriate location under biopandas/docs/sources/ (optional)
  • Checked that the Travis-CI build passed at https://travis-ci.org/rasbt/biopandas

Getting the __version__ during install is more safely done by reading __init__ as text.

The reason is that __init__ may at some point include code that requires importing packages that have not yet been installed. Installing a package should always be possible using `pip install biopython` in an empty virtualenv.
@pep8speaks
Copy link

pep8speaks commented Oct 10, 2019

Hello @ecederstrand! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-10-10 18:09:48 UTC

setup.py Show resolved Hide resolved
@rasbt
Copy link
Member

rasbt commented Oct 10, 2019

Thanks for the PR. Just making sure that I understand the rationale correctly. So, the argument is that if we accumulate a lot of stuff in the __init__.py file, we may have problems getting the version in pip due to import issues?

I am not sure if this is an issue here, because we do not import external packages in the __init__.py

@ecederstrand
Copy link
Contributor Author

It's correct that this isn't an issue right now because the init doesn't contain any import statements. But if you start adding import helpers so you can do biopandas import Foo then you start getting problems. It's quite difficult to debug and only shows up in a fresh virtualenv, so developers usually don't notice before some user complains.

Anyway, I just wanted to raise the issue here since I've wasted countless hours due to this in other projects :-)

setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@rasbt
Copy link
Member

rasbt commented Oct 10, 2019

It's correct that this isn't an issue right now because the init doesn't contain any import statements. But if you start adding import helpers so you can do biopandas import Foo then you start getting problems. It's quite difficult to debug and only shows up in a fresh virtualenv, so developers usually don't notice before some user complains.

Anyway, I just wanted to raise the issue here since I've wasted countless hours due to this in other projects :-)

Yeah, it certainly doesn't hurt to add this. I'd be happy to merge this once the issues are fixed.

@coveralls
Copy link

coveralls commented Oct 10, 2019

Coverage Status

Coverage remained the same at 94.236% when pulling 993f6be on ecederstrand:patch-1 into 83d1b65 on rasbt:master.

@ecederstrand
Copy link
Contributor Author

Wow, I really botched this PR. Sorry. Copy-pasting code and editing files online on GitHub is not a good combination :-)

@rasbt
Copy link
Member

rasbt commented Oct 10, 2019

Looks good, thx! Doc updates etc are not required, because these are just changes under the hood. Will merge. Thanks!

@rasbt rasbt merged commit e84c5e3 into BioPandas:master Oct 10, 2019
@ecederstrand ecederstrand deleted the patch-1 branch October 11, 2019 04:52
nozomu-y pushed a commit to nozomu-y/biopandas that referenced this pull request Jan 9, 2023
* Don't import biopandas during install

Getting the __version__ during install is more safely done by reading __init__ as text.

The reason is that __init__ may at some point include code that requires importing packages that have not yet been installed. Installing a package should always be possible using `pip install biopython` in an empty virtualenv.

* Fix pep8

* Update setup.py

* Fix imports

* Fix for Python 2.7
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.

None yet

4 participants