-
Notifications
You must be signed in to change notification settings - Fork 80
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
make cmdline sigs optional for index #1186
Conversation
…der to account for this change
how should this change be documented? @ctb @luizirber |
Codecov Report
@@ Coverage Diff @@
## latest #1186 +/- ##
==========================================
+ Coverage 84.12% 93.17% +9.05%
==========================================
Files 99 75 -24
Lines 9220 5844 -3376
==========================================
- Hits 7756 5445 -2311
+ Misses 1464 399 -1065
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
this looks great - ok to merge when the tests pass! If you put the magic words "fixes #1066" up there it will also auto-close that issue (not to mention linking to the relevant issue in the first place 😜). please document it in the migration issue over in #1069 - just explain what you did to fix the tests, in suitably general language ;). |
note, tests appear to have failed on py 3.7 on linux. although it looks like it may not be your fault - it's in test_sbt.py, test_sbt_ipfsstorage. |
yeah I saw that but didn't know how to handle - suggestions? |
It seems to be a known error fixed in 3.7.2 (https://bugs.python.org/issue34921). BUT! Travis uses 3.7.1. Sigh. |
Makes the signatures positional arg optional for
sourmash index
. This frees the user to only pass in signatures from a file or load a file as signatures.This change means that the positional arguments need to be placed properly: first the index name, then zero or more signature files. To account for this, I changed the order of args in failing tests using
index
.I also added an index test (
test_index_metagenome_fromfile_no_cmdline_sig
)that does not provide any command line sigs.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.
fixes #1066