bokulich-lab / q2-assembly

QIIME 2 plugin for (meta)genome assembly.
BSD 3-Clause "New" or "Revised" License
4 stars 12 forks source link

fix file name to sample id assumption #38

Closed gregcaporaso closed 1 year ago

gregcaporaso commented 1 year ago

This PR updates an out-dated parameter description (#36) and fixes an error in an assumption about how filenames map onto sample ids (#37). Note that the third commit in this PR fixes this assumption in a second location. That code should ultimately be updated to use the function that was fixed in the second commit, so this logic is centralized (and therefore consistent) throughout the codebase.

Tests also need to be added for the fixed functionality. @misialq or @Keegan-Evans, any chance you could pull these changes and add the test? I have done local testing, and can confirm the that fix in the third commit allows index-contigs to complete for me. The fix in the second commit seems to be working - the job is running, but it's much further along that where it previously failed.

misialq commented 1 year ago

Hey @gregcaporaso, thanks for the PR - I will take over the tests 👍

gregcaporaso commented 1 year ago

Thanks @misialq!

codecov[bot] commented 1 year ago

Codecov Report

Merging #38 (6588660) into main (87b5e02) will increase coverage by 0.00%. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main      #38   +/-   ##
=======================================
  Coverage   98.66%   98.67%           
=======================================
  Files          25       25           
  Lines        1275     1284    +9     
=======================================
+ Hits         1258     1267    +9     
  Misses         17       17           
Impacted Files Coverage Δ
q2_assembly/_action_params.py 100.00% <ø> (ø)
q2_assembly/_utils.py 100.00% <100.00%> (ø)
q2_assembly/bowtie2/tests/test_utils.py 98.97% <100.00%> (+0.05%) :arrow_up:
q2_assembly/bowtie2/utils.py 97.95% <100.00%> (ø)
q2_assembly/tests/test_utils.py 97.70% <100.00%> (+0.11%) :arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

misialq commented 1 year ago

Hey @gregcaporaso, tests were added/modified, in case you wanted to check those out.

misialq commented 1 year ago

All updated, @gregcaporaso!

gregcaporaso commented 1 year ago

Need anything else from me on this one @misialq?

misialq commented 1 year ago

No, all's good - I'm merging now.