Convenience methods for SAM records

[mbhall88]

What are your thoughts on adding some convenience functions to noodles_sam::alignment::Record for common tasks like checking if a record is unmapped, secondary etc.?

I appreciate you might think this is unnecessary bloat given this library seems to be aimed at conciseness and provided others with the ability to add these conveniences themselves easily enough, which is totally fair.

An example of a pattern I use a lot is something like this

let is_secondary = match record.flags() {
    Ok(flags) => flags.is_secondary(),
    Err(_) => false  // I guess it is arguable whether to return false here or return the error
};

It would be great to just be able to call record.is_secondary(). As I said though, I understand if you feel this is overkill for this library.

(I am happy to contribute these too if you feel they would be welcome)

[zaeleus]

Thanks for the suggestion. I would prefer not adding such methods.

An alignment record is represented by its fields, and the flags field encompasses the multitude of boolean options in a record, acting like a namespace. This design avoids having to hoist 12 individual fields, when the flags are better wrapped as its own type and can be manipulated using common bit flag operations. The usage through an obvious bit flags type outweighs the convenience of handling or using many delegators.

Note that your example can be simplified using adapters.

let is_secondary = record.flags().map(|f| f.is_secondary()).unwrap_or(false);

// I guess it is arguable whether to return false here or return the error

But I would indeed argue that the error case should not default to false, as it typically denotes a parse error.

let record = sam::Record::try_from(&b"*\tnoodles\t*\t0\t255\t*\t*\t0\t0\t*\t*"[..])?;
dbg!(record.flags().map(|f| f.is_secondary()).unwrap_or(false));
// => false

The flags field is invalid, and the caller should propagate the error, e.g.,

let is_secondary = record.flags().map(|f| f.is_secondary())?;

Of course, you could always define a helper function if it's being called in many places, e.g.,

fn is_secondary<R>(record: &R) -> io::Result<bool>
where
    R: sam::alignment::Record,
{
    record.flags().map(|f| f.is_secondary())
}

if is_secondary(&record)? {
    // ...
}

Alternatively, you can define your own wrapper or extension trait, e.g.,

trait RecordExt {
    fn is_secondary(&self) -> io::Result<bool>;
}

impl<T> RecordExt for T
where
    T: sam::alignment::Record,
{
    fn is_secondary(&self) -> io::Result<bool> {
        self.flags().map(|f| f.is_secondary())
    }
}

if record.is_secondary()? {
    // ...
}

[mbhall88]

Totally understand. And thank you for the detailed response as always.

Thanks for the adapter example! And yes, I see your point about returning the error being the better option. I have such an extension trait currently, so I will update that.