feat!: bam header to_hashmap method now returns a result in order to deal with header parsing issues

[ArtRand]

I've recently encountered an issue where the to_hashmap method will cause a panic (issue). #276 reports a similar problem. I've refactored the function to return a Result allowing the caller to handle the failure. I understand this changes the API and it may be considered unconventional to have a to_* method return a result, so I'm open to discussion about other ways to aviod the panic.

[tedil]

An alternative would be to implement TryFrom (i.e. impl TryFrom<Header> for HashMap<String, String>), such that

let header_map: HashMap<_, _> = header.try_into()?;

API wise, that would be more difficult to discover, but it does leverage existing traits and clearly signals that something during conversion might fail.

(Also, instead of the if let, one could use a combination of .map and .map_err instead, but that boils down to personal preference I guess ;) )

[veldsla]

This possible panic was already fixed in #384 (released in version 0.43.0) header tags may now be empty.

This is already more lenient than the official spec, it should be an error. Perhaps the unwrap could be replaced with an .expect explaining the file is corrupt/not conforming to specification

[ArtRand]

@tedil

I'm happy to implement it as a TryFrom if you think that's best, I tend to agree that discovery would be difficult.

@veldsla

I appreciate that this particular error would not cause a panic with the new regex pattern. However, we've all experienced out-of-spec data. At the very least I'd expect the method's documentation to mention that it can cause a panic when the header isn't to the spec and (IMO) it's much more ideal to communicate a potential failure with a Result type.

I've refactored this function call out of my project, but would be more than happy to contribute a more reliable solution.

[johanneskoester]

My feeling is that to_hashmap is more ergonomic here. Let's keep it for now. Makes sense to return a result here.