-
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
[MRG] fix tax argument parsing #2218
Conversation
Codecov Report
@@ Coverage Diff @@
## latest #2218 +/- ##
==========================================
+ Coverage 84.76% 92.08% +7.31%
==========================================
Files 131 100 -31
Lines 15619 11350 -4269
Branches 2236 2240 +4
==========================================
- Hits 13240 10452 -2788
+ Misses 2085 608 -1477
+ Partials 294 290 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
7a655cd implemented gotta add a few more tests and this will be fully baked. |
…into fix-tax-cli
This should be ready for review @bluegenes! Note, I took it over from you so github won't care if you approve it, but that's ok, once you like it I'll merge it with admin superpower. |
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.
lgtm, thanks for taking over @ctb!
(gh won't let me approve, but ...I approve :)
Most of the
tax
subcommands take multiple inputs via-g/--gather-csv
and-t/--taxonomy-csv
, but currently the later inputs override earlier inputs, e.g.-g gather1.csv -g gather2.csv
will result in justgather2.csv
being loaded. This PR fixes that by using argparseextend
appropriately, and adds tests throughout.This PR also fixes the
--force
argument so that it is properly passed intoMultiLineageDB.load
for taxonomy loading, and also properly used in gather loading.Fixes #2213
Notes from testing
Testing the argparse
action='extend'
stuff systematically was tricky and annoying. I ended up adding a bunch of tests where I relied on two pieces of behavior ---force
sourmash tax
commands #2213.So I would call something like
sourmash tax prepare ... -t good_tax -t empty_tax --force
.If argparse was set up correctly,
good_tax
would be loaded, andempty_tax
would be ignored.If argparse was set up incorrectly,
good_tax
would be ignored, and onlyempty_tax
would be received, but because it was empty, a later check (for non-empty taxonomies) would fail. Voila!Finally, I verified that if I removed
action='extend'
from argparse options for the-g
and-t
arguments, at least one test failed. 🎉TODO
action= 'extend'
to appropriate argsannotate
prepare
grep
genome
metagenome