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

feat: Add wrapper of BGZF writer #349

Merged
merged 2 commits into from
Jul 5, 2022
Merged

feat: Add wrapper of BGZF writer #349

merged 2 commits into from
Jul 5, 2022

Conversation

jemma-nelson
Copy link
Contributor

@jemma-nelson jemma-nelson commented Mar 20, 2022

This patch exposes BGZF writing capabilities. This is my first time writing unsafe code (outside of tutorials), so some extra attention there would be welcomed. :)

One possibly controversial implementation choice I made:
BGZF supports two different versions of "No Compression." The value -2 / wu results in a "bgzf" writer that simply passes the input through without modifying it - the output file should be the exact same as the input. The value 0 / w0 results in a more normal/usual "Uncompressed" setting, where the output is conforming bgzf format, but doesn't try to compress it.

I chose to expose both of these, calling the first one "NoCompression" and the latter "Uncompressed." These names aren't clear in how they differ, so I would be happy for them to be renamed - or, if the situation is too confusing, removing NoCompression.

If accepted, this PR should close #347.

@jemma-nelson jemma-nelson changed the title Add wrapper of BGZF writer feat: Add wrapper of BGZF writer Mar 20, 2022
Run cargo fmt on src/errors.rs
@brainstorm brainstorm self-requested a review June 29, 2022 23:04
Copy link
Member

@brainstorm brainstorm left a comment

Choose a reason for hiding this comment

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

Thanks for this PR @jemma-nelson. You even seem to take into account testing all levels except NoCompression... is it because of this issue?: zaeleus/noodles#96

@jemma-nelson
Copy link
Contributor Author

I was actually unaware of that issue. I'm not sure if it affects us, since I think htslib might be handling that under the hood.

I have a test for the NoCompression level (in which it writes the contents directly, not BGZF encoded) in fn test_write_plain(), but it could probably have a more obvious name. :)

@brainstorm brainstorm merged commit 965ed88 into rust-bio:master Jul 5, 2022
@jemma-nelson jemma-nelson deleted the feat/bgzf_writer branch July 20, 2022 16:52
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.

Support for BGZF writer?
2 participants