-
Notifications
You must be signed in to change notification settings - Fork 79
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
[MRG] Rename library to sourmash #374
Conversation
52ea5b7
to
06c7e6c
Compare
Codecov Report
@@ Coverage Diff @@
## master #374 +/- ##
==========================================
+ Coverage 90.07% 90.56% +0.49%
==========================================
Files 32 33 +1
Lines 4824 4854 +30
Branches 36 36
==========================================
+ Hits 4345 4396 +51
+ Misses 478 457 -21
Partials 1 1
Continue to review full report at Codecov.
|
1ea5707
to
2f3cdd6
Compare
* sourmash_lib is still available for older scripts * Remove the local sourmash script * Fix tests that expected the local sourmash script
2f3cdd6
to
de49f62
Compare
Ready for review @ctb |
This turned out to be easier than expected. The important lesson is avoiding doing absolute imports inside the module, so instead of doing from sourmash import foo do from . import foo instead. For external/testing code, continue doing either from sourmash import foo or from sourmash_lib import foo |
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.
Looks good to me!
@@ -51,16 +51,16 @@ | |||
"author": "C. Titus Brown", | |||
"author_email": "[email protected]", | |||
"license": "BSD 3-clause", | |||
"packages": ["sourmash_lib"], | |||
"packages": find_packages(), |
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.
Presumably recursively finds all packages and subpackages?
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, it is recommended by the Python packaging guide: https://packaging.python.org/tutorials/distributing-packages/#packages
# NOTE: this script is only used in 'developer' mode (python setup.py | ||
# build_ext -i) -- see setup.py entrypoints for what end-users use. | ||
from sourmash_lib.__main__ import main | ||
main() |
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.
Is there no command line entry point now? Or should we do something like pip install -e .
to set up a dev environment?
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.
You need pip install -e .
(or python setup.py develop
) to have the sourmash
command available (it uses the entry point defined in setup.py
).
On Fri, Mar 09, 2018 at 09:37:36PM -0800, Daniel Standage wrote:
> @@ -1,5 +0,0 @@
-#! /usr/bin/env python
-# NOTE: this script is only used in 'developer' mode (python setup.py
-# build_ext -i) -- see setup.py entrypoints for what end-users use.
-from sourmash_lib.__main__ import main
-main()
Is there no command line entry point now? Or should we do something like `pip install -e .` to set up a dev environment?
yep!
|
great!
can we/should we put in a deprecation warning for 'import sourmash_lib' or
wait until later releases?
|
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.
Looks good!
Note: if developers get an error |
@ctb yes, it is better to 'reset' your environment after this changes (reinstall sourmash with |
yes, exactly.
|
Change library name to sourmash instead of sourmash_lib
Supersedes #164
Fixes #162
make test
Did it pass the tests?make coverage
Is the new code covered?without a major version increment. Changing file formats also requires a
major version number increment.
changes were made?