BCF header records #76
Conversation
| @@ -240,6 +257,54 @@ impl HeaderView { | |||
| (*(*self.inner).id[htslib::BCF_DT_SAMPLE as usize].offset(*id as isize)).key) }; | |||
| key.to_bytes().to_vec() | |||
| } | |||
|
|
|||
| /// Return structured `HeaderRecord`s. | |||
| pub fn header_records(&self) -> Vec<HeaderRecord> { | |||
There was a problem hiding this comment.
I would return a HashMap here, which assigns keys to HeaderRecord.
There was a problem hiding this comment.
I don't think that will be enough. Rust's HashMap type is not insertion order (similar to the dict in Python <3.6). IMO we need to guarantee that (1) insertion order == order in BCF file and (2) vice versa. This is in particularly important for roundtrip processing of VCF files.
There was a problem hiding this comment.
Right. And moreover there might be duplicate keys I think. Returning a Vector is fine.
There was a problem hiding this comment.
Hm, so should I just rebase or perform any changes?
There was a problem hiding this comment.
OK, resolving the merge conflicts now FWIW, would have to be done anyway.
src/bcf/header.rs
Outdated
| /// A header record. | ||
| pub enum HeaderRecord { | ||
| /// A `FILTER` header record. | ||
| Filter { key: String, key_value_pairs: Vec<(String, String)> }, |
There was a problem hiding this comment.
I think these should be <type> {key: String, values: HashMap<String, String>}, except for Generic.
There was a problem hiding this comment.
Wait, this should be LinearMap. By that, insertion order is preseved. https://docs.rs/linear-map
|
🎉 @johanneskoester The test runs through, the code compiles and is properly formatted. Please advise on any further need for change. |
|
@johanneskoester ping |
src/bcf/header.rs
Outdated
| /// A `FILTER` header record. | ||
| Filter { | ||
| key: String, | ||
| key_value_pairs: Vec<(String, String)>, |
There was a problem hiding this comment.
In here, I think a linearmap would be nice. Sorry for the confusion. And I would rename the second field into values.
There was a problem hiding this comment.
OK, did so, let's see what the CI build says.
|
@johanneskoester Well, this runs through in principle but doctests now also fail on stable... Otherwise, I incorporated your changes. What now? |
|
@holtgrewe it should be sufficient to update the |
Cargo.toml
Outdated
| @@ -35,4 +36,4 @@ pretty_assertions = "0.5.1" | |||
|
|
|||
| [build-dependencies] | |||
| fs-utils = "1.0" | |||
| bindgen = "0.35.0" | |||
| bindgen = "0.36.0" | |||
There was a problem hiding this comment.
Without the trailing zero, just 0.36? Then we automatically get patch releases.
|
@johanneskoester No it's not. And now? |
|
This is either a bug in rust or a behavior change in rust and a bug in rust-bindgen. I have created an issue: rust-lang/rust-bindgen#1313 |
|
Can you update to the latest master of rust-htslib? I have temporarily pinned rust to 1.25 |
holtgrewe
force-pushed
the
feature/header-records
branch
from
90ad451
to
05df495
Compare
|
@johanneskoester OK, let's see if the build runs through now. |
johanneskoester
changed the title
|
Yay! Thanks a lot! |
This works but I'm unsure about the design being right.
@johanneskoester what do you think?