-
Notifications
You must be signed in to change notification settings - Fork 53
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 async alignment writer #292
Conversation
For now, convert
Yes, this is intentional. I/O is modeled after the physical structure for the format (see § 5 "File structure" (2024-03-21) for a visual representation). The file header is named to differentiate itself from the many other headers in the format, e.g., the container header, the slice header, the block header, etc. I did add a convenience method to the blocking reader in 2da4eae, which simply needs to be synchronized with the async reader later. Use the currently available methods to write a default file definition, and use the SAM header to write the file header, e.g., noodles/noodles-cram/examples/cram_write_async.rs Lines 73 to 74 in 174ad3c
Good catch; it's definitely a typo. Use the methods that are currently available. I can also fix this after this patch. |
Further to the above, when adding the async Writer example I realised I had missed adding Aside from that, I think this PR is done and ready for review. |
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.
Looks good, thanks!
See #286 for more details.
Checklist
examples/
entryCurrently, I cannot compile the async Writer due to
Which is coming from the fact that
cram::async::io::Writer::write_record
takes acram::Record
type rather than a&dyn sam::alignment::Record
trait.noodles/noodles-cram/src/async/io/writer.rs
Line 184 in 174ad3c
Is that a change that should be made in a separate PR?
In addition, I also noticed some slight differences in function naming schemes between SAM/BAM and CRAM. For example, the SAM and BAM async
Writer
s have awrite_header
function, however, the reciprocal CRAM function iswrite_file_header
- not sure if this was intended though. Also, the CRAM async builder has the functionbuild_with_writer
rather than thebuild_from_writer
. I appreciate these are minor things, but thought I would raise them in case you want to move this library towards consistent names across modules/crates. I guess for backwards compatibility we could add the#[deprecated
attribute if you want to introduce the consistent named version.