Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ scrape descriptions on encode #35

Merged
merged 3 commits into from
Feb 2, 2024

Conversation

dmiller15
Copy link
Contributor

@dmiller15 dmiller15 commented Jan 27, 2024

Followup work to #34.

I tried to tackle the task of INFO field description scraping requested in #33. Found rust-htslib pretty ill-equipped for the task, but I'm way too much of a rust/c novice to delve deeply into the core of htslib. This could perhaps be done better using bcf_hdr_get_hrec.

In the end I made a short function to filter down the HeaderRecord vector using an ID value. Since there should only ever be one INFO header record with a particular ID, I return the description from the first of the remaining records. If it's not there I pass along the default value. On the way out I make sure to trim the " since the update_header function expects the descriptions to be unquoted.

In the encoder_main function I only call this function when the field description is not the default. I figure we'd want to give user priority in field naming.

As for how the check against the default is done, I couldn't really figure out how to get a default value for a field the way things were so I went ahead and made the fields::default_description_string() function public. I'm not sure if that's a good way of doing that so I'm open to suggestions.

Let me know if this is fine. Feel free to discard the PR if you want to go about this a different way.

📝 fix borrows

🐛 fix quote injection
🐛 public func for default checks
@brentp
Copy link
Owner

brentp commented Feb 2, 2024

@dmiller15 , this looks good to me. Ready from your end?

@dmiller15
Copy link
Contributor Author

Just tested it, and it's still working as intended.

@brentp brentp merged commit 6b659aa into brentp:main Feb 2, 2024
1 check passed
@brentp
Copy link
Owner

brentp commented Feb 2, 2024

thank you!

@brentp
Copy link
Owner

brentp commented Feb 2, 2024

@dmiller15 , this fails tests/big.sh. Can you fix that?

@dmiller15
Copy link
Contributor Author

Sure I'll take a look

@dmiller15
Copy link
Contributor Author

Ok didn't notice that we'd added more tests, and the test logic is no longer true. I'll update that batch of tests and have a PR up today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants