rust-htslib: read api changes broke my code :(. / while let Ok(_) = read() no longer works

This is mostly a cautionary tale for others:

The alloc-less way to iterate over reads used to be based on fn read(&mut self, record: &mut record::Record) -> Result<(), ReadError> which has recently (version 0.25) converted to fn read(&mut self, record: &mut record::Record) -> Result<bool>

Accordingly, the calling code most be adjust from while let Ok(_) = bam.read(&mut record) to while let Ok(true) = bam.read(&mut record)

Otherwise you run into an endless loop.

Unfortunatly, the Ok(_) pattern is valid rust even on the new types so no compiler errors.

This has been a fun hour of doubting my sanity.

About this issue

Original URL
State: closed
Created 5 years ago
Comments: 19 (18 by maintainers)

Most upvoted comments

My vote is on ‘drop the read api all together’. It’s a c-ish workaround for a non allocating Iterator.

RcRecords #265 is just as fast in practice, and I’m going to use it even if I have to have it living in an extension

With #261 just being accepted, we could even throw away the two-step API that we inherited from htslib (fetch, then iterate) for a one stop

.records(FetchDefinition) (for when you need copies of the reads anyhow)
.rcrecords(FetchDefinition) (for when you don’t)
.pileup(FetchDefinition) API.

That would be a breaking change I’d consider ok 😃.

TyberiusPrime on Sep 14, 2020

Hmmm, I understand your worries about bike-shedding. But we should come to some conclusion, here.

I see the arguments for keeping it as is (Result<bool>) as:

changing anything, would again be a breaking change and possibly in several of the modules
Result<bool> is being used elsewhere, so is not uncommon
spending time here is time we don’t spend on other issues

I see the arguments for changing this to Option<Result<>>:

Option<Result<>> is less likeley to fail silently (the compiler enforces handling of None)
allows for the while let Some(record) pattern
this pattern could be easier to pick up for people new to Rust

I would still be in favour of changing to Option<Result<>>, especially as this means that the compiler can check for correct usage – which I think is generally the most Rusty way… If we opt for this, we should then implement it wherever we currently have the Result<bool> pattern. But I also don’t mind keeping the Result<bool>, should more people tend towards this, as it can handle all cases. We could still envisage the change for a later API-breaking update.

Any quick votes? Or further arguments? Let’s move this forward!

dlaehnemann on Sep 14, 2020