Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
fix: bam::Record:new should return a valid record #361
fix: bam::Record:new should return a valid record #361
Changes from 5 commits
4db8850
a3278b9
cf5bf1a
cacc1f3
f02c86e
7e3fd68
5ddbd6b
1aaad6f
2b47d10
81013f9
9cd847d
4a51951
d4bc492
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
The goal in this PR is to that one does not need to call any other methods on
Record
to have a valid record. Currently, this requires setting the read name and alignment positions (and unmapped flag if the positions are -1). So this PR has an opinion, that a new record is unmapped with empty name/seq/qual/cigar.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.
Thanks for indirectly making me think about
Default
trait vsnew()
: https://users.rust-lang.org/t/when-to-use-default-trait/11190There 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.
Coincidental good read on Default: https://cj.rs/blog/rust-default-values-for-maintainability/
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.
I would argue that we should also set the record to be unmapped by default. I mean, how can it be mapped if the tid/pos are -1?
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.
Sounds reasonable but I'd quickly compare third party implementations (htslib/htsjdk/noodles) and/or spec footnotes (lots of gotchas there).