feat: implement htslib basemod api

[jts]

This is a rebased implementation of the htslib basemod API that was originally in PR #374.

It relies on changes in rust-bio/hts-sys#3 which will need to be merged first, and a new release made so Cargo.toml can be updated.

cc: @johanneskoester

[johanneskoester]

Very nice and clean work! Thanks a lot!

[jts]

@johanneskoester is there anything else that needs to be done before this is merged?

[dlaehnemann]

Could you run cargo fmt on the branch, so that the CI / Formatting step is happy? As for the linting failures, I'm not sure. Maybe merging in the master branch already helps, as the CI / Linting step seems to have passed on other more recent branches.

With tests passing, I'd be glad to merge this!

[coveralls]

Pull Request Test Coverage Report for Build 5322603414

• 73 of 92 (79.35%) changed or added relevant lines in 1 file are covered.
• 129 unchanged lines in 4 files lost coverage.
• Overall coverage increased (+0.4%) to 79.376%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/bam/record.rs	73	92	79.35%

Files with Coverage Reduction	New Missed Lines	%
src/bam/record.rs	1	76.75%
src/utils.rs	1	66.67%
src/bcf/record.rs	48	78.21%
src/bam/mod.rs	79	82.2%

Totals
Change from base Build 4698271908:	0.4%
Covered Lines:	2417
Relevant Lines:	3045

---

💛 - Coveralls

[jts]

Thanks @dlaehnemann, I have updated the PR to fix the format errors (using cargo fmt as you suggested) and the linter warning. The remaining build errors are because rust-bio/hts-sys#3 needs to be merged and released first, because this PR depends on new bindings.

[dlaehnemann]

The new hts-sys version is released, so I took the liberty of updating the version in Cargo.toml. Hope that's alright for you.

[dlaehnemann]

A CI issue is still holding up the release of the newer version, but this should be resolved soon:
rust-bio/hts-sys#5

[jts]

Thanks for working on this (and for updating Cargo.toml) - let me know if you need any help from me

[dlaehnemann]

Hmm, now it throws a doctest-failure:

---- src/bam/record.rs - bam::record::Record::basemods_iter (line 1025) stdout ----
error[E0425]: cannot find value `record` in this scope
Error:  --> src/bam/record.rs:1026:19
  |
3 | if let Ok(mods) = record.basemods_iter() {
  |                   ^^^^^^ not found in this scope

Probably something you can quickly fix, @jts ?

[jts]

Hm, it seems doctest doesn't like having my example code snippet. I'll see if there's an easy fix, otherwise I'll take the example out.

[dlaehnemann]

Could you instead just initialize a record here? If you think this bloats the example, you could always hide parts of the doctest code that is run.

It would be a shame not to use this doctest...

[jts]

Good idea, I didn't see that feature. PR updated

[dlaehnemann]

This generally looks good, now. Would you feel comfortable and could you find the time to introduce more (doc)-tests?

Not sure how accurate the coveralls report is, but it suggests 92 new code lines uncovered by tests:
https://coveralls.io/builds/60751874

[dlaehnemann]

Ah, and I updated the PR with a change that the previous doctest failure suggested. So before any further changes, a resync with the remote branch probably avoids any conflicts.

[jts]

Sure, I won't be able to get to it today but will try to add some more tests tomorrow.

[jts]

@dlaehnemann I just pushed a commit to see how much of the new code it covers, I'll add more tests if needed later

[jts]

Hmm it seems that doctests aren't counted in the code coverage. Do you know whether that is the case? If so I'll write some proper tests

[dlaehnemann]

It's using tarpaulin with the flag --all-features set. From what I can see, this should also include doctests. But I guess the doctest only covers certain code paths? So maybe unit tests with mod tests also make sense for more systematic testing?

[jts]

I might be misinterpreting the coveralls report but the doctest should go down one code path that is reported as uncovered. No worries though, I'll write some mod tests now instead

[dlaehnemann]

I might be misinterpreting the coveralls report but the doctest should go down one code path that is reported as uncovered. No worries though, I'll write some mod tests now instead

Thanks for checking this, and it's unfortunate that coveralls / tarpaulin don't seem to take the doctests into account, here. But mod tests are a good idea anyways!

[tedil]

I might be misinterpreting the coveralls report but the doctest should go down one code path that is reported as uncovered. No worries though, I'll write some mod tests now instead

Thanks for checking this, and it's unfortunate that coveralls / tarpaulin don't seem to take the doctests into account, here. But mod tests are a good idea anyways!

I think with the current configuration of tarpaulin, doc tests aren't considered at all.
Here's some information/discussion on the choice of parameters needed for tarpaulin to include those: xd009642/tarpaulin#1302

[dlaehnemann]

Ah, thanks for this pointer. I was looking for info like this in the docs and didn't find it. Sounds like we might fix it by adding the --doc --examples flags to our tarpaulin command. Although I am not sure how exactly --all-features (as opposed to the --all-targets mentioned in the issue) handles doctests...

[dlaehnemann]

Looking great, now! Ready to merge, @jts ?

[jts]

Yes, I think I am happy! Thanks so much for all the help getting this PR into shape.

[dlaehnemann]

Well, thanks for all the work!

BTW, I am now trying to get tarpaulin / coveralls to also report on the doctests, over in PR #397.