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

Add unit test coverage of filter.write_vcf and refactor #479

Merged
merged 2 commits into from
Mar 31, 2020

Conversation

elebow
Copy link
Contributor

@elebow elebow commented Mar 26, 2020

Akin to #478, adds unit test coverage and does some nonfunctional refactoring. There's more to do, but not until the module has better coverage.

This also introduces pytest-mock as a development dependency.

#Read in/write out according to file ending
inCall = "--gzvcf" if compressed else "--vcf"
outCall = "| gzip -c" if output_file.lower().endswith('.gz') else ""
def write_vcf(input_filename, output_filename, dropped_samps):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same concern as #478 about changing the API. If this is only used internally it is probably ok, but if they have any users this will cause code breakage.

@huddlej
Copy link
Contributor

huddlej commented Mar 30, 2020

@elebow LGTM! It looks like there are merge conflicts now with your earlier tests, but after you resolve these we can merge this PR. Thank you!

@elebow elebow force-pushed the filter-unit-test-coverage-write_vcf branch from 9b90a76 to a31bcaf Compare March 30, 2020 23:38
@elebow elebow force-pushed the filter-unit-test-coverage-write_vcf branch from a31bcaf to 3097573 Compare March 30, 2020 23:58
@elebow
Copy link
Contributor Author

elebow commented Mar 30, 2020

Rebased and resolved conflicts.

@huddlej huddlej merged commit d5dbc75 into nextstrain:master Mar 31, 2020
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.

3 participants